Re: logical changeset generation v6.7

Hi,

On 2013-12-03 17:13:05 +0900, Kyotaro HORIGUCHI wrote:
> - Some patches have line offset to master. Rebase needed.

Will send the rebased version as soon as I've addressed your comments.

> ===== 0001:
>
> - You assined HeapTupleGetOid(tuple) into relid to read in
> several points but no modification. Nevertheless, you left
> HeapTupleGetOid not replaced there. I think 'relid' just for
> repeated reading has far small merit compared to demerit of
> lowering readability. You'd be better to make them uniform in
> either way.

It's primarily to get the line lengths halfway under control.

> ===== 0002:
>
> - You are identifying the wal_level with the expr 'wal_level >=
> WAL_LEVEL_LOGICAL' but it seems somewhat improper.

Hm. Why?

> - RelationIsAccessibleInLogicalDecoding and
> RelationIsLogicallyLogged are identical in code. Together with
> the name ambiguity, this is quite confising and cause of
> future misuse between these macros, I suppose. Plus the names
> seem too long.

Hm, don't think they are equivalent, rather the contrary. Note one
returns false if IsCatalogRelation(), the other true.

> - In heap_insert, the information conveyed on rdata[3] seems to
> be better to be in rdata[2] because of the scarecity of the
> additional information. XLOG_HEAP_CONTAINS_NEW_TUPLE only
> seems to be needed. Also is in heap_multi_insert and
> heap_upate.

Could you explain a bit more what you mean by that? The reason it's a
separate rdata entry is that otherwise a full page write will remove the
information.

> - In heap_multi_insert, need_cids referred only once so might be
> better removed.

It's accessed in a loop over potentially quite some items, that's why I
moved it into an extra variable.

> - In heap_delete, at the point adding replica identity, same to
> the aboves, rdata[3] could be moved into rdata[2] making new
> type like 'xl_heap_replident'.

Hm. I don't think that'd be a good idea, because we'd then need special
case decoding code for deletes because the wal format would be different
for inserts/updates and deletes.

> - In heapam_xlog.h, the new type xl_heap_header_len is
> inadequate in both of naming which is confising and
> construction on which the header in xl_heap_header is no
> longer be a header and indecisive member name 't_len'..

The "header" bit in the name refers to the fact that it's containing
information about the a HeapTuple's header, not that it's a header
itself. Do you have a better suggestion than xl_heap_header_len?

> - In heapam_xlog.h, XLOG_HEAP_CONTAINS_OLD looks incomplete. And
> it seems to be used in nowhere in this patchset. It should be
> removed.

Not sure what you mean with incomplete? It contains the both possible
variants for an old contained tuple. The macro is used in the decoding,
but I don't think things get clearer if we revise the macros in that
later patch.

> - log_heap_new_cid() is called at several part just before other
> xlogs is being inserted. I suppose this should be built in the
> target xlog structures.

Proportionally it will only be logged in a absolute minority of the
cases (since normally the catalog will only seldomly be updated in
comparison to a user's tables), so it doesn't seem like a good idea to
complicate the already *horribly* complicated wal format for heap_*.

> - in RecovoerPreparedTransactions(), any commend needed for the
> reason calling XLogLogicalInfoActive()..

It's pretty much the "Test here must match one used in
AssignTransactionId()" comment. We only want to allow overwriting if
AssignTransactionId() might already have done the SubTransSetParent()
calls.

> - In xact.c, the comment for the member 'didLogXid' in
> TransactionStateData seems differ from it's meaning. It
> becomes true when any WAL record for the current transaction
> id just has been written to WAL buffer. So the comment,
>
> > /* has xid been included in WAL record? */
>
> would be better be something like (Should need corrected as
> I'm not native speaker.)

> /* Any WAL record for this transaction has been emitted ? */

I don't think that'd be an improvement, transaction is a bit ambigiuous
there because it might be the toplevel or subtransaction.

> and also the member name should be something like
> XidIsLogged. (Not so chaned?)

Hm.

> - The name of the function MarkCurrentTransactionIdLoggedIfAny,
> although irregular abbreviations are discouraged, seems too
> long. Isn't MarkCur(r/rent)XidLoggedIfAny sufficient?

If you look at the other names in xact.h that doesn't seem to fit too
well in the naming pattern.

> Anyway,
> the work involving this function seems would be better to be
> done in some other way..

Why? How?

> - The comment for RelationGetIndexAttrBitmap() should be edited
> for attrKind.

Good point.

> - The macro name INDEX_ATTR_BITMAP_KEY should be
> INDEX_ATTR_BITMAP_FKEY. And INDEX_ATTR_BITMAP_IDENTITY_KEY
> should be INDEX_ATTR_BITMAP_REPLID_KEY or something.

But INDEX_ATTR_BITMAP_KEY isn't just about foreign keys... But I agree
that INDEX_ATTR_BITMAP_IDENTITY_KEY should be renamed.

Greetings,

Andres Freund

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