rename pg_log_standby_snapshot

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

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.

I don't really agree with this. I think breaking compatibility in the next
major release
makes sense here. We already have pg_log_backend_memory_contexts writing to
the log, and
If we end up adding a few more functions called pg_log_something that write
to the log file,
pg_log_standby_snapshot will stand out awkwardly.

--
Sami Imseih
Amazon Web Services (AWS)