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

On 2013-07-22 13:50:08 -0400, Robert Haas wrote:
> On Fri, Jun 14, 2013 at 6:51 PM, Andres Freund <andres@2ndquadrant.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
significantlyspeed up   startup of physical replicas. Ony many systems checkpoint_timeout   will be set to an hour
whichcan make the time till a standby gets   consistent be quite high since that will be first time it sees a
xl_running_xactsagain.
 

> 0011: make fsync_fname() public; required, needs to be in a different file   Isn't in the shape for it atm, but could
beapplied 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
uselesswork.
 


> 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