RE: Logical Replication of sequences

From: "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>
To: 'vignesh C' <vignesh21(at)gmail(dot)com>
Cc: Nisha Moond <nisha(dot)moond412(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, shveta malik <shveta(dot)malik(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com>, Peter Eisentraut <peter(at)eisentraut(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Euler Taveira <euler(at)eulerto(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, "Jonathan S(dot) Katz" <jkatz(at)postgresql(dot)org>
Subject: RE: Logical Replication of sequences
Date: 2025-08-01 10:47:53
Message-ID: OSCPR01MB14966325AA3494AF164E11B9CF526A@OSCPR01MB14966.jpnprd01.prod.outlook.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Dear Vignesh,

I played with your patch and found something.

01.
In LogicalRepSyncSequences() and GetSubscriptionRelations(), there is a possibility
that the sequence on the subscriber could be dropped before opens that.
This can cause `could not open relation with OID %u` error, which is not user-friendly.
Can we avoid that? Even if it is difficult we should add ereport().

02.
```
/*
* Check that our sequencesync worker has permission to insert into
* the target sequence.
*/
aclresult = pg_class_aclcheck(RelationGetRelid(sequence_rel), GetUserId(),
ACL_INSERT);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult,
get_relkind_objtype(sequence_rel->rd_rel->relkind),
seqname);
```

Hmm, but upcoming SetSequence() needs UPDATE privilege. I feel it should be checked.

03.
Similar with 1, sequences can be dropped just before entering copy_sequences().
This can cause `cache lookup failed for sequence` error, which cannot be translated.
Can we avoid that or change the error-function to erport()?

04.
```
if (message_level_is_interesting(DEBUG1))
ereport(DEBUG1,
errmsg_internal("logical replication synchronization for subscription \"%s\", sequence \"%s.%s\" has finished",
MySubscription->name,
seqinfo->nspname,
seqinfo->seqname));
```

I feel no need to add if-statement because we do not touch additional data here.

05.
```
list_free_deep(sequences_to_copy);
```

IIUC, this function free's each elements and list itself, but they do no-op for
attributes of elements. Can we pfree() for seqname and nspname?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2025-08-01 10:50:38 Re: Conflict detection for update_deleted in logical replication
Previous Message Vik Fearing 2025-08-01 10:34:02 Re: implement CAST(expr AS type FORMAT 'template')