On Tue, Feb 9, 2021 at 12:02 PM Peter Smith <smithpb2...@gmail.com> wrote: > > Here are my feedback comments for the V29 patch. > > ==== > > FILE: logical-replication.sgml > > + slots have generated names: > <quote><literal>pg_%u_sync_%u_%llu</literal></quote> > + (parameters: Subscription <parameter>oid</parameter>, > + Table <parameter>relid</parameter>, system > identifier<parameter>sysid</parameter>) > + </para> > > 1. > There is a missing space before the sysid parameter. > > ===== > > FILE: subscriptioncmds.c > > + * SUBREL_STATE_FINISHEDCOPY. The apply worker can also > + * concurrently try to drop the origin and by this time the > + * origin might be already removed. For these reasons, > + * passing missing_ok = true from here. > + */ > + snprintf(originname, sizeof(originname), "pg_%u_%u", sub->oid, relid); > + replorigin_drop_by_name(originname, true, false); > + } > > 2. > Don't really need to say "from here". > (same comment applies multiple places, in this file and in tablesync.c) > > 3. > Previously the tablesync origin name format was encapsulated in a > common function. IMO it was cleaner/safer how it was before, instead > of the same "pg_%u_%u" cut/paste and scattered in many places. > (same comment applies multiple places, in this file and in tablesync.c) >
Fixed all the three above comments. > 4. > Calls like replorigin_drop_by_name(originname, true, false); make it > unnecessarily hard to read code when the boolean params are neither > named as variables nor commented. I noticed on another thread [et0205] > there was an idea that having no name/comments is fine because anyway > it is not difficult to figure out when using a "modern IDE", but since > my review tools are only "vi" and "meld" I beg to differ with that > justification. > (same comment applies multiple places, in this file and in tablesync.c) > Already responded to it separately. I went ahead and removed such comments from other places in the patch. > [et0205] > https://www.postgresql.org/message-id/c1d9833f-eeeb-40d5-89ba-87674e1b7ba3%40www.fastmail.com > > ===== > > FILE: tablesync.c > > 5. > Previously there was a function tablesync_replorigin_drop which was > encapsulating the tablesync origin name formatting. I thought that was > better than the V29 code which now has the same formatting scattered > over many places. > (same comment applies for worker_internal.h) > I am not sure what different you are expecting here than point-3? > + * Determine the tablesync slot name. > + * > + * The name must not exceed NAMEDATALEN - 1 because of remote node > constraints > + * on slot name length. We do append system_identifier to avoid slot_name > + * collision with subscriptions in other clusters. With current scheme > + * pg_%u_sync_%u_UINT64_FORMAT (3 + 10 + 6 + 10 + 20 + '\0'), the maximum > + * length of slot_name will be 50. > + * > + * The returned slot name is either: > + * - stored in the supplied buffer (syncslotname), or > + * - palloc'ed in current memory context (if syncslotname = NULL). > + * > + * Note: We don't use the subscription slot name as part of tablesync slot > name > + * because we are responsible for cleaning up these slots and it could become > + * impossible to recalculate what name to cleanup if the subscription slot > name > + * had changed. > + */ > +char * > +ReplicationSlotNameForTablesync(Oid suboid, Oid relid, char > syncslotname[NAMEDATALEN]) > +{ > + if (syncslotname) > + sprintf(syncslotname, "pg_%u_sync_%u_" UINT64_FORMAT, suboid, relid, > + GetSystemIdentifier()); > + else > + syncslotname = psprintf("pg_%u_sync_%u_" UINT64_FORMAT, suboid, relid, > + GetSystemIdentifier()); > + > + return syncslotname; > +} > > 6. > "We do append" --> "We append" > "With current scheme" -> "With the current scheme" > Fixed. > 7. > Maybe consider to just assign GetSystemIdentifier() to a static > instead of calling that function for every slot? > static uint64 sysid = GetSystemIdentifier(); > IIUC the sysid value is never going to change for a process, right? > Already responded. > FILE: alter_subscription.sgml > > 8. > + <para> > + Commands <command>ALTER SUBSCRIPTION ... REFRESH ..</command> and > + <command>ALTER SUBSCRIPTION ... SET PUBLICATION ..</command> with refresh > + option as true cannot be executed inside a transaction block. > + </para> > > My guess is those two lots of double dots ("..") were probably meant > to be ellipsis ("...") > Fixed, for the first one I completed the command by adding PUBLICATION. > > When looking at the DropSubscription code I noticed that there is a > small difference between the HEAD code and the V29 code when slot_name > = NONE. > > HEAD does > ------ > if (!slotname) > { > table_close(rel, NoLock); > return; > } > ------ > > V29 does > ------ > if (!slotname) > { > /* be tidy */ > list_free(rstates); > return; > } > ------ > > Isn't the V29 code missing doing a table_close(rel, NoLock) there? > Yes, good catch. Fixed. -- With Regards, Amit Kapila.
v30-0001-Make-pg_replication_origin_drop-safe-against-con.patch
Description: Binary data
v30-0002-Allow-multiple-xacts-during-table-sync-in-logica.patch
Description: Binary data
