On Thu, Feb 2, 2023 at 5:01 PM shveta malik <shveta.ma...@gmail.com> wrote: > > Reviewing further.... >
Few more comments for v10-0002 and v7-0001: 1) + * need_full_snapshot + * if true, create a snapshot able to read all tables, + * otherwise do not create any snapshot. + * CreateDecodingContext(..,CreateDecodingContext,..) --Is the comment correct? Shall we have same comment here as that of 'CreateDecodingContext' * need_full_snapshot -- if true, must obtain a snapshot able to read all * tables; if false, one that can read only catalogs is acceptable. This function is not going to create a snapshot anyways. It is just a pre-step and then the caller needs to call 'SnapBuild' functions to build a snapshot. Here need_full_snapshot decides whether we need all tables or only catalog tables changes only and thus the comment change is needed. ========== 2) Can we please add more logging: 2a) when lastusedId is incremented and updated in pg_* table ereport(DEBUG2, (errmsg("[subid:%d] Incremented lastusedid to:%ld",MySubscription->oid, MySubscription->lastusedid))); Comments for LogicalRepSyncTableStart(): 2b ) After every UpdateSubscriptionRel: ereport(DEBUG2, (errmsg("[subid:%d] LogicalRepSyncWorker_%ld updated origin to %s and slot to %s for relid %d", MyLogicalRepWorker->subid, MyLogicalRepWorker->rep_slot_id, originname, slotname, MyLogicalRepWorker->relid))); 2c ) After walrcv_create_slot: ereport(DEBUG2, (errmsg("[subid:%d] LogicalRepSyncWorker_%ld created slot %s", MyLogicalRepWorker->subid, MyLogicalRepWorker->rep_slot_id, slotname))); 2d) After replorigin_create: ereport(DEBUG2, (errmsg("[subid:%d] LogicalRepSyncWorker_%ld created origin %s [id: %d]", MyLogicalRepWorker->subid, MyLogicalRepWorker->rep_slot_id, originname, originid))); 2e) When it goes to reuse flow (i.e. before walrcv_slot_snapshot), if needed we can dump newly obtained origin_startpos also: ereport(DEBUG2, (errmsg("[subid:%d] LogicalRepSyncWorker_%ld reusing slot %s and origin %s", MyLogicalRepWorker->subid, MyLogicalRepWorker->rep_slot_id, slotname, originname))); 2f) Also in existing comment: + (errmsg("logical replication table synchronization worker for subscription \"%s\" has moved to sync table \"%s\".", + MySubscription->name, get_rel_name(MyLogicalRepWorker->relid)))); we can add relid also along with relname. relid is the one stored in pg_subscription_rel and thus it becomes easy to map. Also we can change 'logical replication table synchronization worker' to 'LogicalRepSyncWorker_%ld'. Same change needed in other similar log lines where we say that worker started and finished. Please feel free to change the above log lines as you find appropriate. I have given just a sample sort of thing. thanks Shveta