Re: logical changeset generation v6.7
От | Kyotaro HORIGUCHI |
---|---|
Тема | Re: logical changeset generation v6.7 |
Дата | |
Msg-id | 20131205.220351.189609364.horiguchi.kyotaro@lab.ntt.co.jp обсуждение исходный текст |
Ответ на | Re: logical changeset generation v6.7 (Andres Freund <andres@2ndquadrant.com>) |
Ответы |
Re: logical changeset generation v6.7
(Andres Freund <andres@2ndquadrant.com>)
|
Список | pgsql-hackers |
Hello, > Will send the rebased version as soon as I've addressed your comments. Thank you. > > ===== 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. Mm. I'm afraid I couldn't catch your words, do you mean that IsSystemClass() or isTempNamespace() could change the NULL bitmap in the tuple? > > ===== 0002: > > > > - You are identifying the wal_level with the expr 'wal_level >= > > WAL_LEVEL_LOGICAL' but it seems somewhat improper. > > Hm. Why? It actually does no harm and somewhat trifling so I don't assert you should fix it. The reason for the comment is the greater values for wal_level are undefined at the moment, so strictly saying, such values should be handled as invalid ones. Although there is a practice to avoid loop overruns by comparing counters with the expression like (i > CEILING). For instance, I found a macro for which comment reads as follows and I feel a bit uneasy with it :-) It's nothing more than that. | /* Do we need to WAL-log information required only for Hot Standby? */ ~~~~ | #define XLogStandbyInfoActive() (wal_level >= WAL_LEVEL_HOT_STANDBY) > > - 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. Oops, I'm sorry. I understand they are not same. Then I have other questions. The name for the first one 'RelationIsAccessibleInLogicalDecoding' doesn't seem representing what its comment reads. | /* True if we need to log enough information to have access via | decoding snapshot. */ Making the macro name for this comment directly, I suppose it would be something like 'NeedsAdditionalInfoInLogicalDecoding' or more directly 'LogicalDeodingNeedsCids' or so.. > > - 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. Sorry, I missed the comment 'so that an eventual FPW doesn't remove the tuple's data'. Although given the necessity of removal prevention, rewriting rdata[].buffer which is required by design (correct?) with InvalidBuffer seems a bit outrageous for me and obfuscating the objective of it. Other mechanism should be preferable, I suppose. The most straight way to do that should be new flag bit for XLogRecData, say, survive_fpw or something. > > - 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. Sorry bothering you with comments biside the point.. But the scope of needs_cids is narrower than it is. I think the definition should be moved into the block for 'if (needwal)'. > > - 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. Hmm. Although one common xl_heap_replident is impractical, splitting a logcally single entity into two or more XLogRecDatas also seems not to be a good idea. > > - 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? Sorry, I'm confused during writing the comment, The order of members in xl_heap_header_len doesn't matter. I got the reason for the xl_header_len and whole xlog record image after re-reading the relevant code. The update record became to contain two variable length data by this patch. So the length of the tuple body cannot be calculated only with whole record length and header lengths. Considering above, looking heap_xlog_insert(), you marked on xlrec.flags with XLOG_HEAP_CONTAINS_NEW_TUPLE to signal decoder that the record should have tuple data not being removed by fpw. This is the same for the update record. So the redoer(?) also can distinguish whether the update record contains extra tuple data or not. On the other hand, the update record after patched is longer by sizeof(uint16) regardless of whether 'tuple data' is attached or not. I don't know the value of the size in WAL stream, but the smaller would be better maybe. As a conclusion, I thing it would be better to decide whether to insert length SEGMENT before the tuple data segment in log_heap_update. Like follows, | rdata[1].next = &(rdata[2]); | | xlhdr.t_infomask2 = newtup->t_data->t_infomask2; | xlhdr.t_infomask = newtup->t_data->t_infomask; | xlhdr.t_hoff = newtup->t_data->t_hoff; | | /*...*/ | rdata[2].data = (char *) &xlhdr; | ... | rdata[2].next = &(rdata[3]); | | if (need_tuple_data) | { | uint16 newtupbodylen = | newtup->t_len - offsetof(HeapTupleHeaderData, t_bits); | rdata[3].data = &newtupbodylen; | .... | } | else | { | rdata[3].data = NULL; | rdata[3].len = 0; | ... | } | rdata[3].next = &(rdata[4]); | | /* PG73FGORMAT: write bitmap [+ padding] [+ oid] + data */ | rdata[4].data = (char *) newtup->t_data; > > - 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. Umm. I don't understand why I missed where it is used, it surely used in decode.c as you mentioned. Ok, this is required. Then about 'imcomplete', it means 'CONTAINS OLD what?'...mmm, logically speaking, lack of an object for the word 'OLD'. The answer for the question should be 'both KEY and TUPLE'. Comparing the revised images, # Of course, it doesn't matter if the object for OLD can # naturally be missing as English. I'm not a native English # speaker as you know:-) defining XLOG_HEAP_CONTAINS_OLD_KEY_AND_TUPLE, | if (xlrec->flags & XLOG_HEAP_CONTAINS_OLD_KEY_AND_TUPLE) | { undefining XLOG_HEAP_CONTAINS_OLD and use separte macros type 1 | if (xlrec->flags & XLOG_HEAP_CONTAINS_OLD_KEY || | xlrec->flags & XLOG_HEAP_CONTAINS_OLD_TUPLE) | { (I belive this should be optimized by the compiler:-) and type 2 | if (xlrec->flags & | (XLOG_HEAP_CONTAINS_OLD_KEY | XLOG_HEAP_CONTAINS_OLD_TUPLE)) | { I'm ok with any of them or others. In this connection, I found following phrase in heapam.c which like type2 above. | if (!(old_infomask & (HEAP_XMAX_INVALID | | HEAP_XMAX_COMMITTED | | HEAP_XMAX_IS_MULTI)) && > > - 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_*. Horribly:-) I agree to you. Hmm I reconsider with new knowledge(!) about your patch. But what do you think doing this as follows, if (RelationIsAccessibleInLogicalDecoding(relation)) + { | rdata_heap_new_cid(&rdata[0], relation, heaptup); + xlrec.flags |= XLOG_HEAP_CONTAINS_NEW_CID; + } + else + rdata_void(&rdata[0]) + rdata[0].next = &(rdata[1]); xlrec.flags = all_visible_cleared ? XLOG_HEAP_ALL_VISIBLE_CLEARED : 0; xlrec.target.node =relation->rd_node; xlrec.target.tid = heaptup->t_self; | rdata[1].data = (char *) &xlrec; If you don't agree with this, I don't say no more about this. > > - 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. Thank you, I found it and it seems to be sufficient. > > - 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. Hmm. Ok, I agree with it. > > 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. (Now looking...) Wow. Ok, I agree to you. > > Anyway, > > the work involving this function seems would be better to be > > done in some other way.. > > Why? How? How... It is easy to comment but hard to realize. Ok, let's forget this comment:-) The 'Why' came from some obscure impression(?), without firm thought. > > - The comment for RelationGetIndexAttrBitmap() should be edited > > for attrKind. > > Good point. Thanks. > > - 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. Thank you, please remember to do that. -- Kyotaro Horiguchi NTT Open Source Software Center
В списке pgsql-hackers по дате отправления:
Следующее
От: Heikki LinnakangasДата:
Сообщение: Re: Performance optimization of btree binary search