Horton

Re: Skipping logical replication transactions on subscriber side

Masahiko Sawada <sawada.mshk@gmail.com>

View thread View on postgr.es
From: Masahiko Sawada <sawada.mshk@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>, vignesh C <vignesh21@gmail.com>, Greg Nancarrow <gregn4422@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>, Alexey Lesovsky <lesovsky@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2022-01-24T02:57:59Z
Lists: pgsql-hackers 
On Fri, Jan 21, 2022 at 8:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jan 21, 2022 at 10:10 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Fri, Jan 21, 2022 at 1:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > What do we want to indicate by [, ... ]? To me, it appears like
> > > multiple options but that is not what we support currently.
> >
> > You're right. It's an oversight.
> >
>
> I have fixed this and a few other things in the attached patch.

Thank you for updating the patch!

> 1.
> The newly added column needs to be updated in the following statement:
> -- All columns of pg_subscription except subconninfo are publicly readable.
> REVOKE ALL ON pg_subscription FROM public;
> GRANT SELECT (oid, subdbid, subname, subowner, subenabled, subbinary,
>               substream, subtwophasestate, subslotname, subsynccommit,
> subpublications)
>     ON pg_subscription TO public;
>
> 2.
> +stop_skipping_changes(bool clear_subskipxid, XLogRecPtr origin_lsn,
> +   TimestampTz origin_timestamp)
> +{
> + Assert(is_skipping_changes());
> +
> + ereport(LOG,
> + (errmsg("done skipping logical replication transaction %u",
> + skip_xid)));
>
> Isn't it better to move this LOG at the end of this function? Because
> clear* functions can give an error, so it is better to move it after
> that. I have done that in the attached.
>
> 3.
> +-- fail - must be superuser
> +SET SESSION AUTHORIZATION 'regress_subscription_user2';
> +ALTER SUBSCRIPTION regress_testsub SKIP (xid = 100);
> +ERROR:  must be owner of subscription regress_testsub
>
> This test doesn't seem to be right. You want to get the error for the
> superuser but the error is for the owner. I have changed this test to
> do what it intends to do.
>
> Apart from this, I have changed a few comments and ran pgindent. Do
> let me know what you think of the changes?

Agree with these changes.

>
> Few things that I think we can improve in 028_skip_xact.pl are as follows:
>
> After CREATE SUBSCRIPTION, wait for initial sync to be over and
> two_phase state to be enabled. Please see 021_twophase.

Agreed.

> For the
> streaming case, we might be able to ensure streaming even with lesser
> data. Can you please try that?

Yeah, after some tests, it's enough to insert 500 rows as follows:

INSERT INTO test_tab_streaming SELECT i, md5(i::text) FROM
generate_series(1, 500) s(i);

I've just sent another email about that probably we can remove two
tests for ROLLBACK PREPARED, so I’ll update the patch while including
this point.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Commits

  1. Test ALIGNOF_DOUBLE==4 compatibility under ALIGNOF_DOUBLE==8.

  2. Reorder subskiplsn in pg_subscription to avoid alignment issues.

  3. Add ALTER SUBSCRIPTION ... SKIP.

  4. Optionally disable subscriptions on error.

  5. Update docs of logical replication for commit 8d74fc96db.

  6. Respect permissions within logical replication.

  7. Fix regression test failure caused by commit 8d74fc96db.

  8. Add a view to show the stats of subscription workers.

  9. Add logical change details to logical replication worker errcontext.

  10. Rename LOGICAL_REP_MSG_STREAM_END to LOGICAL_REP_MSG_STREAM_STOP.

  11. Fix typo in protocol.sgml.

  12. Remove unused argument in apply_handle_commit_internal().

  13. Fix replication of in-progress transactions in tablesync worker.

  14. Reorder pg_sequence columns to avoid alignment issue