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