Re: Re[2]: [PATCH] Add pg_current_vxact_id() function to expose virtual transaction IDs - Mailing list pgsql-hackers

Hi Pavlo,

Thank you for the v2 patch. I've reviewed it and here are my comments:

== Summary ==

The patch applies cleanly and all regression tests pass.
The implementation is straightforward and follows existing patterns.

== Detailed Review ==

1. Functionality: OK
   - The function correctly returns the VXID in the expected format.

2. Tests: OK
   - Regression tests are included and pass.
   - gcov/valgrind testing is unnecessary due to the simplicity of the code.

3. Code Safety: OK
   - Buffer size (32 bytes) is sufficient for maximum output (23 bytes),
     consistent with VXIDGetDatum() in lockfuncs.c.
   - Memory allocated by cstring_to_text() via palloc is in
     ecxt_per_tuple_memory and automatically managed.

4. Typos: None found.

== Suggestions for Improvement ==

1. Format String Duplication

   The format string "%d/%u" is now duplicated in three places:
   - src/backend/utils/adt/lockfuncs.c (VXIDGetDatum)
   - src/backend/utils/error/elog.c (%v placeholder)
   - src/backend/utils/adt/xid8funcs.c (pg_current_vxact_id)

   Consider defining a macro in lock.h for consistency:

     #define VXID_FMT "%d/%u"

   All three files already include lock.h indirectly:
   - lockfuncs.c -> predicate_internals.h -> lock.h
   - elog.c -> proc.h -> lock.h
   - xid8funcs.c -> proc.h -> lock.h

2. Documentation Terminology

   The terms "localTransactionId" and "localXID" are used inconsistently:
   - localTransactionId: 30+ in C code (actual field name), 1 in sgml (monitoring.sgml)
   - localXID: 3 in sgml only (xact.sgml, config.sgml)

   The new func-info.sgml uses "localTransactionId" which matches the
   actual C struct field name. However, existing documentation prefers
   "localXID" for user-facing text. Consider using "localXID" in
   func-info.sgml for consistency with xact.sgml and config.sgml.

== Comparison with pg_current_xact_id ==

The implementation follows a similar pattern to pg_current_xact_id(),
which was introduced in commit 4c04be9b05a. The placement in
xid8funcs.c is appropriate.