Horton

Thread

Commits

  1. Rename pg_sequence_read_tuple() to pg_get_sequence_data()

  1. Removing log_cnt from pg_sequence_read_tuple()

    Michael Paquier <michael@paquier.xyz> — 2024-08-26T02:11:55Z

    Hi all,
(Nathan in CC regarding c8b06bb969bf)

While rebasing my patch set for sequence AMs, I've looked at what has
been done with c8b06bb969bf and pg_sequence_read_tuple() because I can
reuse it in the sequence AM patch to grab the last value of a sequence
and if it has been called (the patch set implemented the same thing,
with a different function name), and got surprised that the function
also returns log_cnt, which is for the in-core sequence metadata an
internal counter to decide when a sequence should be WAL-logged.

Why do we need this field at all in this function?  pg_dump only cares
about the last value and is_called to be able to rebuilt its sequence
DDLs, and log_cnt is reset each time we restore or do a crash
recovery, bumping at the next sequence value based on an internal of
32.

I am also a bit dubious about the value it adds for debugging.  The
thing is that depending on the way sequences are computed, we may not
care about WAL at all, and log_cnt is something related to the way
in-core sequences are computed and how its data is persistent.  So
this makes the whole concept of sequence metadata a bit fuzzier
because we mix data necessary for the sequence command and more
things.  There is no need for it in pg_dump or pg_upgrade, either.

last_value and is_called are different and required all the time,
of course, because these define how the sequence DDLs should be
created.

It seems to me that we'd live better without it, at least it matters
for the sequence AM patch, because some of its callbacks are also
shaped around the fact that WAL may not be required for sequence value
computations.  Providing a function that should be rather generic does
not fit well in this context, still I agree that it comes down to how
the callbacks are defined, of course.  My point is that the use of WAL
is something that should be optional, but log_cnt in this function
makes it a mandatory concept that all sequence AMs would need to deal
with.

Comments?
--
Michael

  2. Re: Removing log_cnt from pg_sequence_read_tuple()

    Nathan Bossart <nathandbossart@gmail.com> — 2024-08-26T14:19:06Z

    On Mon, Aug 26, 2024 at 11:11:55AM +0900, Michael Paquier wrote:
> It seems to me that we'd live better without it, at least it matters
> for the sequence AM patch, because some of its callbacks are also
> shaped around the fact that WAL may not be required for sequence value
> computations.  Providing a function that should be rather generic does
> not fit well in this context, still I agree that it comes down to how
> the callbacks are defined, of course.  My point is that the use of WAL
> is something that should be optional, but log_cnt in this function
> makes it a mandatory concept that all sequence AMs would need to deal
> with.

I am fine with changes to this function that would allow it to be
generically useful for all sequence AMs, provided that it can still be used
for dumpSequenceData().  I only included log_cnt because
pg_sequence_read_tuple() is intended to be a substitute for SELECT from the
sequence, but I'm not aware of any real use-case for that column besides
debugging, which you've already addressed.  Even if we remove log_cnt, you
can still find it with SELECT, too.

The patch looks reasonable to me.  Do you think the name of the function
still makes sense now that 1) we might have different sequence AMs in the
near future and 2) it no longer returns everything in the sequence tuple?

-- 
nathan

  3. Re: Removing log_cnt from pg_sequence_read_tuple()

    Michael Paquier <michael@paquier.xyz> — 2024-08-28T23:00:52Z

    On Mon, Aug 26, 2024 at 09:19:06AM -0500, Nathan Bossart wrote:
> I am fine with changes to this function that would allow it to be
> generically useful for all sequence AMs, provided that it can still be used
> for dumpSequenceData().  I only included log_cnt because
> pg_sequence_read_tuple() is intended to be a substitute for SELECT from the
> sequence, but I'm not aware of any real use-case for that column besides
> debugging, which you've already addressed.

Okay, thanks.

> Even if we remove log_cnt, you can still find it with SELECT, too.

The design used in the sequence AM patch makes it possible to assign
custom attributes to the sequence "relation" used for storage, with a
table AM used underneath that may not be heap.  The AM callback
plugged into the path used by pg_sequence_read_tuple() (previous
version for pg_sequence_last_value) returns the pair is_called and
last_value, to map to the row of the function used to rebuild the
commands in dumps and upgrades.

> The patch looks reasonable to me.  Do you think the name of the function
> still makes sense now that 1) we might have different sequence AMs in the
> near future and 2) it no longer returns everything in the sequence tuple?

Indeed, pg_sequence_read_tuple() would not reflect the reality, some
ideas: 
- pg_sequence_read_data
- pg_sequence_get_data
- pg_sequence_data
- More consistent with other catalog functions: pg_get_sequence_data,
as we have already in the tree a lot of pg_get_* functions.
--
Michael

  4. Re: Removing log_cnt from pg_sequence_read_tuple()

    Nathan Bossart <nathandbossart@gmail.com> — 2024-08-29T01:28:03Z

    On Thu, Aug 29, 2024 at 08:00:52AM +0900, Michael Paquier wrote:
> On Mon, Aug 26, 2024 at 09:19:06AM -0500, Nathan Bossart wrote:
>> The patch looks reasonable to me.  Do you think the name of the function
>> still makes sense now that 1) we might have different sequence AMs in the
>> near future and 2) it no longer returns everything in the sequence tuple?
> 
> Indeed, pg_sequence_read_tuple() would not reflect the reality, some
> ideas: 
> - pg_sequence_read_data
> - pg_sequence_get_data
> - pg_sequence_data
> - More consistent with other catalog functions: pg_get_sequence_data,
> as we have already in the tree a lot of pg_get_* functions.

pg_get_sequence_data() sounds fine to me.

-- 
nathan

  5. Re: Removing log_cnt from pg_sequence_read_tuple()

    Michael Paquier <michael@paquier.xyz> — 2024-08-29T05:11:22Z

    On Wed, Aug 28, 2024 at 08:28:03PM -0500, Nathan Bossart wrote:
> On Thu, Aug 29, 2024 at 08:00:52AM +0900, Michael Paquier wrote:
>> Indeed, pg_sequence_read_tuple() would not reflect the reality, some
>> ideas: 
>> - pg_sequence_read_data
>> - pg_sequence_get_data
>> - pg_sequence_data
>> - More consistent with other catalog functions: pg_get_sequence_data,
>> as we have already in the tree a lot of pg_get_* functions.
> 
> pg_get_sequence_data() sounds fine to me.

Okay, here is a v2 of the patch using this name for the function.
--
Michael

  6. Re: Removing log_cnt from pg_sequence_read_tuple()

    Nathan Bossart <nathandbossart@gmail.com> — 2024-08-29T14:28:49Z

    On Thu, Aug 29, 2024 at 02:11:22PM +0900, Michael Paquier wrote:
> Okay, here is a v2 of the patch using this name for the function.

LGTM

-- 
nathan

  7. Re: Removing log_cnt from pg_sequence_read_tuple()

    Michael Paquier <michael@paquier.xyz> — 2024-08-29T23:59:31Z

    On Thu, Aug 29, 2024 at 09:28:49AM -0500, Nathan Bossart wrote:
> On Thu, Aug 29, 2024 at 02:11:22PM +0900, Michael Paquier wrote:
> > Okay, here is a v2 of the patch using this name for the function.
> 
> LGTM

Thanks, applied that, after one tweak for the #define name.
--
Michael