Re: changeset generation v5-01 - Patches & git tree - Mailing list pgsql-hackers

Andres Freund 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

I gave this recently rebased branch a skim.  In general, the separation 
between decode.c/reorderbuffer.c/snapbuild.c seems a lot nicer now than
on previous iterations -- good job there.

Here are some quick notes I took while reading the patch itself.  I
haven't gone through it really carefully, yet.


- I wonder whether DecodeCommit and DecodeAbort should really be a single routine.  Right now, the former might call
thelater; and the latter is aware of this.  Seems awkward.
 

- We skip insert/update/delete if not my database Id; however, we don't skip commit in the same case.  If there are two
walrecvrson a cluster, on different databases, does this lead to us trying to remove files twice, if a xact commits
whichdeleted some files?  Is this a problem? Should we try to skip such database-specific actions in global WAL
records?

- There's rmgr-specific knowledge in decode.c.  I wonder if, similar to redo and desc routines, that shouldn't instead
bepluggable functions for each rmgr.
 

- What's with ReorderBufferRestoreCleanup()?  Shouldn't it be in logical.c?

- reorderbuffer.c does several different things.  Can it be split? Perhaps in pieces such as * stuff to manage memory
(slabcache thingies) * TXN iterator * other logically separate parts? * the rest
 

- Having to expose LocalExecuteInvalidationMessage() looks awkward.  Is there another way?

- I think we need a better name for "treat_as_catalog_table" (and RelationIsTreatedAsCatalogTable).  Maybe
replication_catalogor something similar?
 

- Don't do this: + * RecentGlobal(Data)?Xmin is initialized to InvalidTransactionId, to ensure that no because later
grepsfor RecentGlobalDataXmin and RecentGlobalXmin will fail to find it.  It seems better to spell both names, so
"RecentGlobalDataXminand RecentGlobalXmin are initialized to ..."
 

- the pg_receivellog command line is strange.  Apparently I need one or more of --start,--init,--stop, but if stop,
thenthe other two must not be present; and if startpos, then init and stop cannot be specified.  (There's a typo there
thatsays "cannot combine with --start" when it really means "cannot combine with --stop", BTW).  I think this would
makemore sense to have init,start,stop be commands, in pg_ctl's spirit; so there would be no double-dash.  IOW
SOMEPATH/pg_receivellog--startpos=123 start and so on.  Also, we need SGML docs for this new utility.
 


Any particular reason for removing this line:
-/* Get a new XLogReader */
+extern XLogReaderState *XLogReaderAllocate(XLogPageReadCB pagereadfunc,                  void *private_data);


Typo here (2n*d*Quadrant):
+= Snapshot Building =
+:author: Andres Freund, 2nQuadrant Ltd



I don't see the point of XLogRecordBuffer.record_data; we already have a
pointer to the XLogRecord, and the data can readily be obtained using
XLogRecGetData.  So why provide the same thing twice?  It seems to me
that if instead of passing the XLogRecordBuffer we just provide the
XLogRecord, and separately the "origptr" where needed, we could avoid
having to expose the XLogRecordBuffer stuff unnecessarily.


In this comment:
+ * FIXME: We need something resembling the real SnapshotNow to handle things
+ * like enum lookups from indices correctly.
what do we need consider in light of the new comment proposed by Robert
CA+TgmobvTjRj_doXxQ0wgA1a1JLYPVYqtR3m+Cou_ousabnmXg@mail.gmail.com

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services