Re: changeset generation v5-01 - Patches & git tree

From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>, Abhijit Menon-Sen <ams(at)2ndQuadrant(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: changeset generation v5-01 - Patches & git tree
Date: 2013-07-23 08:51:28
Message-ID: 20130723085128.GF21996@alap2.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 2013-07-22 13:50:08 -0400, Robert Haas wrote:
> On Fri, Jun 14, 2013 at 6:51 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > The git tree is at:
> > git://git.postgresql.org/git/users/andresfreund/postgres.git branch xlog-decoding-rebasing-cf4
> > http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/xlog-decoding-rebasing-cf4
> >
> > On 2013-06-15 00:48:17 +0200, Andres Freund wrote:
> >> Overview of the attached patches:
> >> 0001: indirect toast tuples; required but submitted independently
> >> 0002: functions for testing; not required,
> >> 0003: (tablespace, filenode) syscache; required
> >> 0004: RelationMapFilenodeToOid: required, simple
> >> 0005: pg_relation_by_filenode() function; not required but useful
> >> 0006: Introduce InvalidCommandId: required, simple
> >> 0007: Adjust Satisfies* interface: required, mechanical,
> >> 0008: Allow walsender to attach to a database: required, needs review
> >> 0009: New GetOldestXmin() parameter; required, pretty boring
> >> 0010: Log xl_running_xact regularly in the bgwriter: required
> >> 0011: make fsync_fname() public; required, needs to be in a different file
> >> 0012: Relcache support for an Relation's primary key: required
> >> 0013: Actual changeset extraction; required
> >> 0014: Output plugin demo; not required (except for testing) but useful
> >> 0015: Add pg_receivellog program: not required but useful
> >> 0016: Add test_logical_decoding extension; not required, but contains
> >> the tests for the feature. Uses 0014
> >> 0017: Snapshot building docs; not required
>
> I've now also committed patch #7 from this series. My earlier commit
> fulfilled the needs of patches #3, #4, and #5; and somewhat longer ago
> I committed #1.

Thanks!

> I am not entirely convinced of the necessity or
> desirability of patch #6, but as of now I haven't studied the issues
> closely.

Fair enough. It's certainly possible to work around not having it, but
it seems cleaner to introduce the notion of an invalid CommandId like we
have for transaction ids et al.
Allowing 2^32-2 instead of 2^32-1 subtransactions doesn't seem like a
problem to me ;)

> Patch #2 does not seem useful in isolation; it adds new
> regression-testing stuff but doesn't use it anywhere.

Yes. I found it useful to test stuff around making replication
synchronous or such, but while I think we should have a facility like it
in core for both, logical and physical replication, I don't think this
patch is ready for prime time due to it's busy looping. I've even marked
it as such above ;)
My first idea to properly implement that seems to be to reuse the
syncrep infrastructure but that doesn't look trivial.

> I doubt that any of the remaining patches (#8-#17) can be applied
> separately without understanding the shape of the whole patch set, so
> I think I, or someone else, will need to set aside more time for
> detailed review before proceeding further with this patch set. I
> suggest that we close out the CommitFest entry for this patch set one
> way or another, as there is no way we're going to get the whole thing
> done under the auspices of CF1.

Generally agreed. The biggest chunk of the code is in #13 anyway...

Some may be applyable independently:

> 0010: Log xl_running_xact regularly in the bgwriter: required
Should be useful independently since it can significantly speed up
startup of physical replicas. Ony many systems checkpoint_timeout
will be set to an hour which can make the time till a standby gets
consistent be quite high since that will be first time it sees a
xl_running_xacts again.

> 0011: make fsync_fname() public; required, needs to be in a different file
Isn't in the shape for it atm, but could be applied as an
independent infrastructure patch. And it should be easy enough to
clean it up.

> 0012: Relcache support for an Relation's primary key: required
Might actually be a good idea independently as well. E.g. the
materalized key patch could use the information that there's a
candidate key around to avoid a good bit of useless work.

> I'll try to find some more time to spend on this relatively soon, but
> I think this is about as far as I can take this today.

Was pretty helpful already, so ... ;)

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Albe Laurenz 2013-07-23 09:53:27 Re: LDAP: bugfix and deprecated OpenLDAP API
Previous Message Andres Freund 2013-07-23 08:30:02 Re: Back branches vs. gcc 4.8.0