Re: Fix publisher-side sequence permission reporting

Hi,

On Tue, Jun 23, 2026 at 5:41 PM Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> BTW, isn't the current documentation a bit misleading? It says:
>
> This function returns a row of NULL values if the sequence does not exist.
>
> But if the specified object does not exist, pg_get_sequence_data()
> raises an error rather than returning a row of NULL values. On the
> other hand, it does return a row of NULL values if the specified
> object exists but is not a sequence. Is my understanding correct? If
> so, how about something like:

When the provided relation oid doesn't exist, returns NULL:
postgres=# select pg_get_sequence_data(99999999);
pg_get_sequence_data
----------------------
(,,)
(1 row)

When the provided relation oid (pg_statistic) exists but not a
sequence, returns NULL:
postgres=# select pg_get_sequence_data(2619);
pg_get_sequence_data
----------------------
(,,)
(1 row)

When the provided relation oid exists and is a sequence, returns sequence data:
postgres=# select pg_get_sequence_data(16388);
pg_get_sequence_data
----------------------
(1,f,0/017DD9A0)
(1 row)

> > > Do you mean that sequencesync.c should also distinguish other
> > > sessions' temporary sequences and unlogged sequences on standbys, and
> > > report separate warnings for those cases?
> > >
> >
> > Right. I mean to ask if we want to distinguish the lack of privilege
> > as a separate case then why not others? My opinion on this point is
> > that improving all these cases (including lack of privileges) together
> > could be considered as an enhancement for the next version but if you
> > think this is sort of a must to distinguish one or more cases then we
> > can do it now as well. However, the reason for doing it now is not
> > clear to me.
>
> I think the lack-of-privilege case should be checked first, since it is
> likely to be the most common one.
>
> As for another session's temporary sequences, I don't think a sequence
> sync worker can actually encounter one. To do so, it would have to
> specify another session's temporary namespace when executing the query
> below. However, the namespace comes from the publication, and temporary
> sequences are never published, so that doesn't seem possible.

Ideally, all these checks (except seqrel being NULL when
try_relation_open detects a concurrent drop) should report an ERROR if
not met, instead of silently emitting NULLs, so the subscriber catches
these early and reports "could not fetch sequence information from the
publisher:" But I understand that on the subscriber side it would
require error message parsing to distinguish the exact cause, which is
not ideal.

However, instead of pg_get_sequence_data emitting just 3 columns, it
could emit additional columns such as:

"last_value", "is_called", "page_lsn", "has_privileges",
"is_sequence", "is_temp_relation", "is_recovery_in_progress"

When any of the new flag columns are true, the first three values
would be NULLs.

This keeps the subscriber-side query simple and leaves the existing
error-handling code as-is.

> > > > errhint("Grant SELECT on the sequence to the replication role on the
> > > > publisher.")
> > >
> > > How about making this a bit more precise?
> > >
> > > errhint("Grant SELECT on the sequence to the role used for the
> > > replication connection on the publisher.")
> > >
> >
> > makes sense.
>
> The attached patch adds those HINT messages. It also updates the
> related documentation.

Nice! This looks more explicit and clarifying. The patch LGTM.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com