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

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 the later; 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 walrecvrs on a cluster, on
different databases, does this lead to us trying to remove files
twice, if a xact commits which deleted 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 be pluggable 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 (slab cache 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_catalog or
something similar?

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

- the pg_receivellog command line is strange. Apparently I need one or
more of --start,--init,--stop, but if stop, then the other two must
not be present; and if startpos, then init and stop cannot be
specified. (There's a typo there that says "cannot combine with
--start" when it really means "cannot combine with --stop", BTW). I
think this would make more 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(at)mail(dot)gmail(dot)com

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