Re: Fix publisher-side sequence permission reporting

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Bharath Rupireddy <[email protected]>
To: Fujii Masao <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Tristan Partin <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Fix publisher-side sequence permission reporting
Date: Tue, 23 Jun 2026 22:40:44 -0700
Message-ID: <CALj2ACVAEfVznEv5ubjQJAjDvwwgt0U1qh+s-+hBW0vBapHcEQ@mail.gmail.com> (raw)
In-Reply-To: <CAHGQGwHuuffj_js4n2g2M07vpkbun8Aj_4+o4ALK=rQpBfwk7w@mail.gmail.com>
References: <CAHGQGwGNTaXnBKUV510_P1KwhdbHT+kgZ4zU5njBHy7nCqdhzg@mail.gmail.com>
	<[email protected]>
	<CAHGQGwE95GLD7RZr2g=zTHKJRafKACmKouhb4TTVH155Hy8zKA@mail.gmail.com>
	<[email protected]>
	<CAHGQGwFLXCAxKEyvvaTLadHZ8k4DFmjJopnN+Vy-NjtfKoM7dw@mail.gmail.com>
	<CAA4eK1JOo0aJRhFHNWpj3hMwaTtNOopY34f1Lh_QD=z=+DrzWQ@mail.gmail.com>
	<CAHGQGwEA6PMAE0aMx4MFqj9d+q2Q835gfUyTSJjAkoMwtWB77A@mail.gmail.com>
	<CAA4eK1+C+mG6ihu_Cs3KjMw9DJgUneSXy5k0HsXyoE1EeWQ1gQ@mail.gmail.com>
	<CAHGQGwHuuffj_js4n2g2M07vpkbun8Aj_4+o4ALK=rQpBfwk7w@mail.gmail.com>

Hi,

On Tue, Jun 23, 2026 at 5:41 PM Fujii Masao <[email protected]> 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

view thread (20+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Fix publisher-side sequence permission reporting
  In-Reply-To: <CALj2ACVAEfVznEv5ubjQJAjDvwwgt0U1qh+s-+hBW0vBapHcEQ@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox