Thread: rename pg_log_standby_snapshot

Hi,

While looking at [1] which introduces a new function called pg_log_query_plan to
write an explain plan to the log file, I noticed that we currently
have overloaded
the meaning of the "pg_log_" prefix.

Currently there is pg_log_backend_memory_contexts which logs memory
contexts to the log file, but there is also a pg_log_standby_snapshot  which
takes a snapshot of running transactions and writes them to wal, so it has
nothing to do with writing to the log file.

                                      List of functions
   Schema   |              Name              | Result data type |
Argument data types | Type
------------+--------------------------------+------------------+---------------------+------
 pg_catalog | pg_log_backend_memory_contexts | boolean          |
integer             | func
 pg_catalog | pg_log_standby_snapshot        | pg_lsn           |
               | func
(3 rows)

Should the pg_log_ prefix strictly refer to functions that write to
logs? If so, should we rename
pg_log_standby_snapshot to something else, such as
pg_standby_snapshot_xip_to_wal?
This name is long, but it is still shorter than other system function
names and clearly describes
what the function does.

Additionally, this name is similar to pg_snapshot_xip, which returns
in-progress transactions.

Also, pg_snapshot_ is a good prefix for future functions that operate
on standbys only.

Another minor thing: the documentation for pg_log_standby_snapshot
function currently says,
"Take a snapshot of running transactions and write it to WAL."
However, we commonly refer to these transactions as "in-progress transactions."
So, maybe the documentation should say, "Take a snapshot of
in-progress transactions and write it to WAL."


[1]
https://www.postgresql.org/message-id/flat/0e0e7ca08dff077a625c27a5e0c2ef0a%40oss.nttdata.com#6539312988e4695d2d416688a3ab34fa

--
Sami Imseih
Amazon Web Services (AWS)

On Thu, Apr 3, 2025 at 9:30 PM Sami Imseih <samimseih@gmail.com> wrote:
>
> While looking at [1] which introduces a new function called pg_log_query_plan to
> write an explain plan to the log file, I noticed that we currently
> have overloaded
> the meaning of the "pg_log_" prefix.
>
> Currently there is pg_log_backend_memory_contexts which logs memory
> contexts to the log file, but there is also a pg_log_standby_snapshot  which
> takes a snapshot of running transactions and writes them to wal, so it has
> nothing to do with writing to the log file.
>
>                                       List of functions
>    Schema   |              Name              | Result data type |
> Argument data types | Type
> ------------+--------------------------------+------------------+---------------------+------
>  pg_catalog | pg_log_backend_memory_contexts | boolean          |
> integer             | func
>  pg_catalog | pg_log_standby_snapshot        | pg_lsn           |
>                | func
> (3 rows)
>
> Should the pg_log_ prefix strictly refer to functions that write to
> logs?
>

I don't know how strict we should be about this, but your question has
merit and deserves some bike-shedding because the user can get
confused with the term *_log_* in the function name that intends to
write a WAL record for standby_snapshot.

> If so, should we rename
> pg_log_standby_snapshot to something else, such as
> pg_standby_snapshot_xip_to_wal?
>

The other option could be pg_create_standby_snapshot(), which would be
similar to the existing function pg_create_restore_point(). I think we
need to think about backward compatibility if we agree with moving in
this direction.

--
With Regards,
Amit Kapila.

Hi,

On Fri, Apr 04, 2025 at 04:10:19PM +0530, Amit Kapila wrote:
> On Thu, Apr 3, 2025 at 9:30 PM Sami Imseih <samimseih@gmail.com> wrote:
> >
> > While looking at [1] which introduces a new function called pg_log_query_plan to
> > write an explain plan to the log file, I noticed that we currently
> > have overloaded
> > the meaning of the "pg_log_" prefix.
> >
> > Currently there is pg_log_backend_memory_contexts which logs memory
> > contexts to the log file, but there is also a pg_log_standby_snapshot  which
> > takes a snapshot of running transactions and writes them to wal, so it has
> > nothing to do with writing to the log file.
> >
> >                                       List of functions
> >    Schema   |              Name              | Result data type |
> > Argument data types | Type
> > ------------+--------------------------------+------------------+---------------------+------
> >  pg_catalog | pg_log_backend_memory_contexts | boolean          |
> > integer             | func
> >  pg_catalog | pg_log_standby_snapshot        | pg_lsn           |
> >                | func
> > (3 rows)
> >
> > Should the pg_log_ prefix strictly refer to functions that write to
> > logs?
> >
> 
> I don't know how strict we should be about this,

I don't know as well and specially given that:

- the snapshot is logged to the log file (if log level <= DEBUG2)
- the doc explains what the function does
- that name also makes sense from an API point of view as it calls "LogStandbySnapshot"

> The other option could be pg_create_standby_snapshot(), which would be
> similar to the existing function pg_create_restore_point(). I think we
> need to think about backward compatibility if we agree with moving in
> this direction.

+1

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

> > > Should the pg_log_ prefix strictly refer to functions that write to
> > > logs?
> > >
> >
> > I don't know how strict we should be about this,
>
> I don't know as well and specially given that:
>
> - the snapshot is logged to the log file (if log level <= DEBUG2)

But unlike pg_log_backend_memory_contexts, the primary purpose
of this function is not to write at the LOG message level.

> - that name also makes sense from an API point of view as it calls "LogStandbySnapshot"

I don't really see the correlation between the user facing pg_log_
prefix and the Log prefixed
functions that write to wal.

But this goes back to the main point of should pg_log_ be specific to
functions that
write to the server logs only. I am making the argument that we
should. We have a precedent
with pg_stat_ being the prefix for any function related to the cumulative stats.

I think it keeps things nicely organized and just overall good code
hygiene, but also not sure
how we can even enforce such naming conventions.

> > The other option could be pg_create_standby_snapshot(), which would be
> > similar to the existing function pg_create_restore_point(). I think we
> > need to think about backward compatibility if we agree with moving in
> > this direction.
>
> +1

This is slightly better. pg_create_restore_point also writes to wal and _create_
has a more generic meaning.


--
Sami Imseih
Amazon Web Services (AWS)

Hi,

On 2025-04-04 11:55:41 -0500, Sami Imseih wrote:
> > > > Should the pg_log_ prefix strictly refer to functions that write to
> > > > logs?
> > > >
> > >
> > > I don't know how strict we should be about this,
> >
> > I don't know as well and specially given that:
> >
> > - the snapshot is logged to the log file (if log level <= DEBUG2)
> 
> But unlike pg_log_backend_memory_contexts, the primary purpose
> of this function is not to write at the LOG message level.
> 
> > - that name also makes sense from an API point of view as it calls "LogStandbySnapshot"
> 
> I don't really see the correlation between the user facing pg_log_
> prefix and the Log prefixed
> functions that write to wal.
> 
> But this goes back to the main point of should pg_log_ be specific to
> functions that
> write to the server logs only. I am making the argument that we
> should. We have a precedent
> with pg_stat_ being the prefix for any function related to the cumulative stats.
> 
> I think it keeps things nicely organized and just overall good code
> hygiene, but also not sure
> how we can even enforce such naming conventions.

I think this would all be a nice argument to have when introducing a new
function. But I don't think it's a wart sufficiently big to justify breaking
compatibility.

Greetings,

Andres Freund

On Sat, Apr 05, 2025 at 10:32:40PM -0400, Andres Freund wrote:
> I think this would all be a nice argument to have when introducing a new
> function. But I don't think it's a wart sufficiently big to justify breaking
> compatibility.

Yeah, I would side as well with the compatibility argument on this
one.
--
Michael