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

>  - 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()

>  - 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?)


>  - 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
>    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.


Andres Freund

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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to