On Fri, Mar 25, 2022 at 3:29 AM Tomas Vondra <tomas.von...@enterprisedb.com> wrote: > > Pushed. >
Some of the comments given by me [1] don't seem to be addressed or responded to. Let me try to say again for the ease of discussion: * Don't we need some syncing mechanism between apply worker and sequence sync worker so that apply worker skips the sequence changes till the sync worker is finished, otherwise, there is a risk of one overriding the values of the other? See how we take care of this for a table in should_apply_changes_for_rel() and its callers. If we don't do this for sequences for some reason then probably a comment somewhere is required. * Don't we need explicit privilege checking before applying sequence data as we do in commit a2ab9c06ea15fbcb2bfde570986a06b37f52bcca for tables? Few new comments: ================= 1. A simple test like the below crashes for me: postgres=# create sequence s1; CREATE SEQUENCE postgres=# create sequence s2; CREATE SEQUENCE postgres=# create publication pub1 for sequence s1, s2; server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. The connection to the server was lost. Attempting reset: Failed. 2. In apply_handle_sequence() do we need AccessExclusiveLock for non-transactional case? 3. In apply_handle_sequence(), I think for transactional case, we need to skip the operation, if the skip lsn is set. See how we skip in apply_handle_insert() and similar functions. [1] - https://www.postgresql.org/message-id/CAA4eK1Jn-DttQ%3D4Pdh9YCe1w%2BzGbgC%2B0uR0sfg2RtkjiAPmB9g%40mail.gmail.com -- With Regards, Amit Kapila.
