On Tue, Oct 12, 2021 at 7:58 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > > On Tue, Oct 12, 2021 at 4:00 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > I've attached updated patches. > > > > Some comments for the v16-0003 patch:
Thank you for the comments! > > (1) doc/src/sgml/logical-replication.sgml > > The output from "SELECT * FROM pg_stat_subscription_errors;" still > shows "last_failed_time" instead of "last_error_time". Fixed. > > doc/src/sgml/ref/alter_subscription.sgml > (2) > > Suggested update (and fix typo: restrited -> restricted): > > BEFORE: > + Setting and resetting of <literal>skip_xid</literal> option is > + restrited to superusers. > AFTER: > + The setting and resetting of the > <literal>skip_xid</literal> option is > + restricted to superusers. Fixed. > > (3) > Suggested improvement to the wording: > > BEFORE: > + incoming change or by skipping the whole transaction. This option > + specifies transaction ID that logical replication worker skips to > + apply. The logical replication worker skips all data modification > AFTER: > + incoming changes or by skipping the whole transaction. This option > + specifies the ID of the transaction whose application is to > be skipped > + by the logical replication worker. The logical replication worker > + skips all data modification Updated. > > (4) src/backend/replication/logical/worker.c > > Suggested improvement to the comment wording: > > BEFORE: > + * Stop the skipping transaction if enabled. Otherwise, commit the changes > AFTER: > + * Stop skipping the transaction changes, if enabled. Otherwise, > commit the changes Fixed. > > > (5) skip_xid value validation > > The validation of the specified skip_xid XID value isn't great. > For example, the following value are accepted: > > ALTER SUBSCRIPTION sub SET (skip_xid='123abcz'); > ALTER SUBSCRIPTION sub SET (skip_xid='99$@*'); Hmm, this is probably a problem of xid data type. For example, we can do like: postgres(1:12686)=# select 'aa123'::xid; xid ----- 0 (1 row) postgres(1:12686)=# select '123aa'::xid; xid ----- 123 (1 row) It seems a problem to me. Perhaps we can fix it in a separate patch. What do you think? Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
