Обсуждение: updates for handling optional argument in system functions

Hi everyone,

I noticed how it was preferred to define optional arguments with the
system functions in system_functions.sql instead of defining them in
pg_proc.dat.

I took a gross stab at updating the ones that ended in _ext, which
turned out to be 7 declarations across 6 system functions, and created a
patch per system function, hoping it would be easier to review.

Perhaps the most interesting thing to share is the total reduction of
the lines of code, although system_functions.sql only grows:

 src/backend/catalog/system_functions.sql |  49 ++++++++
 src/backend/utils/adt/ruleutils.c        | 130 ----------------------
 src/include/catalog/pg_proc.dat          |  36 ++----
 3 files changed, 56 insertions(+), 159 deletions(-)


Is that something we want?

Regards,
Mark
--
Mark Wong <markwkm@gmail.com>
EDB https://enterprisedb.com

> On 10 Dec 2025, at 00:28, Mark Wong <markwkm@gmail.com> wrote:

> Is that something we want?

I have yet to study the patch, but conceptually I am favour of this.  I find
reading the code is easier when it's done this way.

--
Daniel Gustafsson

Hi everyone,

On Tue, Dec 09, 2025 at 03:28:59PM -0800, Mark Wong wrote:
> Hi everyone,
> 
> I noticed how it was preferred to define optional arguments with the
> system functions in system_functions.sql instead of defining them in
> pg_proc.dat.
> 
> I took a gross stab at updating the ones that ended in _ext, which
> turned out to be 7 declarations across 6 system functions, and created a
> patch per system function, hoping it would be easier to review.
> 
> Perhaps the most interesting thing to share is the total reduction of
> the lines of code, although system_functions.sql only grows:
> 
>  src/backend/catalog/system_functions.sql |  49 ++++++++
>  src/backend/utils/adt/ruleutils.c        | 130 ----------------------
>  src/include/catalog/pg_proc.dat          |  36 ++----
>  3 files changed, 56 insertions(+), 159 deletions(-)
> 
> 
> Is that something we want?

I fixed an error caught by the address sanitizer in CI [1] and am uploading a
new patchset.  The only change is to 2 lines in
v2-0005-Handle-pg_get_expr-default-args-in-system_functio.patch to update a
call to pg_get_expr with the correct number of arguments in tablecmds.c.

Regards,
Mark

[1] https://cirrus-ci.com/task/6109065824174080
--
Mark Wong <markwkm@gmail.com>
EDB https://enterprisedb.com

> On Jan 21, 2026, at 02:15, Mark Wong <markwkm@gmail.com> wrote:
>
> Hi everyone,
>
> On Tue, Dec 09, 2025 at 03:28:59PM -0800, Mark Wong wrote:
>> Hi everyone,
>>
>> I noticed how it was preferred to define optional arguments with the
>> system functions in system_functions.sql instead of defining them in
>> pg_proc.dat.
>>
>> I took a gross stab at updating the ones that ended in _ext, which
>> turned out to be 7 declarations across 6 system functions, and created a
>> patch per system function, hoping it would be easier to review.
>>
>> Perhaps the most interesting thing to share is the total reduction of
>> the lines of code, although system_functions.sql only grows:
>>
>> src/backend/catalog/system_functions.sql |  49 ++++++++
>> src/backend/utils/adt/ruleutils.c        | 130 ----------------------
>> src/include/catalog/pg_proc.dat          |  36 ++----
>> 3 files changed, 56 insertions(+), 159 deletions(-)
>>
>>
>> Is that something we want?
>
> I fixed an error caught by the address sanitizer in CI [1] and am uploading a
> new patchset.  The only change is to 2 lines in
> v2-0005-Handle-pg_get_expr-default-args-in-system_functio.patch to update a
> call to pg_get_expr with the correct number of arguments in tablecmds.c.
>
> Regards,
> Mark
>
> [1] https://cirrus-ci.com/task/6109065824174080
> --
> Mark Wong <markwkm@gmail.com>
> EDB https://enterprisedb.com
>
<v2-0001-Handle-pg_get_ruledef-default-args-in-system_func.patch><v2-0002-Handle-pg_get_viewdef-default-args-in-system_func.patch><v2-0003-Handle-pg_get_indexdef-default-args-in-system_fun.patch><v2-0004-Handle-pg_get_constraintdef-default-args-in-syste.patch><v2-0005-Handle-pg_get_expr-default-args-in-system_functio.patch><v2-0006-Handle-pg_get_triggerdef-default-args-in-system_f.patch>

Hi Mark,

Thanks for the patch. After reviewing it, I have mixed feelings. From one side, it removes some redundant code, which
isgood. In the other side, I doubt if we should delete proc entries from pg_proc.c? Say, there is a view that uses a
procto be deleted, the proc OID is stored with the view, then after an upgrade, the view would be broken. From this
perspective,should we retain the old proc entries and only point them to the new functions? 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Wed, Jan 21, 2026 at 04:45:35PM +0800, Chao Li wrote:
> 
> 
> > On Jan 21, 2026, at 02:15, Mark Wong <markwkm@gmail.com> wrote:
> > 
> > Hi everyone,
> > 
> > On Tue, Dec 09, 2025 at 03:28:59PM -0800, Mark Wong wrote:
> >> Hi everyone,
> >> 
> >> I noticed how it was preferred to define optional arguments with the
> >> system functions in system_functions.sql instead of defining them in
> >> pg_proc.dat.
> >> 
> >> I took a gross stab at updating the ones that ended in _ext, which
> >> turned out to be 7 declarations across 6 system functions, and created a
> >> patch per system function, hoping it would be easier to review.
> >> 
> >> Perhaps the most interesting thing to share is the total reduction of
> >> the lines of code, although system_functions.sql only grows:
> >> 
> >> src/backend/catalog/system_functions.sql |  49 ++++++++
> >> src/backend/utils/adt/ruleutils.c        | 130 ----------------------
> >> src/include/catalog/pg_proc.dat          |  36 ++----
> >> 3 files changed, 56 insertions(+), 159 deletions(-)
> >> 
> >> 
> >> Is that something we want?
> > 
> > I fixed an error caught by the address sanitizer in CI [1] and am uploading a
> > new patchset.  The only change is to 2 lines in
> > v2-0005-Handle-pg_get_expr-default-args-in-system_functio.patch to update a
> > call to pg_get_expr with the correct number of arguments in tablecmds.c.
> > 
> > Regards,
> > Mark
> > 
> > [1] https://cirrus-ci.com/task/6109065824174080
> 
> Hi Mark,
> 
> Thanks for the patch. After reviewing it, I have mixed feelings. From one side, it removes some redundant code, which
isgood. In the other side, I doubt if we should delete proc entries from pg_proc.c? Say, there is a view that uses a
procto be deleted, the proc OID is stored with the view, then after an upgrade, the view would be broken. From this
perspective,should we retain the old proc entries and only point them to the new functions?
 

I don't have a solution for the case of a view storing the OID, but Álvaro
Herrera suggested to me to at least try preventing those OIDs from being
reused.

I've attached a v3 patch set that introduces src/include/catalog/pg_retired.dat
to store previously used OIDs and procedure names that the scripts unused_oids
and renumber_oids.pl can consume to prevent the reuse of retired OIDs.

Maybe that can also be used towards finding that particular solution...

Regards,
Mark
-- 
Mark Wong <markwkm@gmail.com>
EDB https://enterprisedb.com

On 1/30/26 1:15 AM, Mark Wong wrote:
>> Thanks for the patch. After reviewing it, I have mixed feelings. From one side, it removes some redundant code,
whichis good. In the other side, I doubt if we should delete proc entries from pg_proc.c? Say, there is a view that
usesa proc to be deleted, the proc OID is stored with the view, then after an upgrade, the view would be broken. From
thisperspective, should we retain the old proc entries and only point them to the new functions?
 
> 
> I don't have a solution for the case of a view storing the OID, but Álvaro
> Herrera suggested to me to at least try preventing those OIDs from being
> reused.
> 
> I've attached a v3 patch set that introduces src/include/catalog/pg_retired.dat
> to store previously used OIDs and procedure names that the scripts unused_oids
> and renumber_oids.pl can consume to prevent the reuse of retired OIDs.
> 
> Maybe that can also be used towards finding that particular solution...
I am not sure what can be done, breaking people's databases on 
pg_upgrade is certainly not nice and detecting that function oid has 
been used anywhere in a database sounds painful, especially since there 
are no references to system oids in pg_depend, right?

That said this patch should be updated to use the new support for 
default values in BKI files.[1]

https://git.postgresql.org/cgit/postgresql.git/commit/?id=759b03b24ce96f0ba6d734b570d1a6f4a0fb1177

Andreas

On Wed, Feb 18, 2026 at 10:56:56PM +0100, Andreas Karlsson wrote:
> On 1/30/26 1:15 AM, Mark Wong wrote:
> > > Thanks for the patch. After reviewing it, I have mixed feelings. From one side, it removes some redundant code,
whichis good. In the other side, I doubt if we should delete proc entries from pg_proc.c? Say, there is a view that
usesa proc to be deleted, the proc OID is stored with the view, then after an upgrade, the view would be broken. From
thisperspective, should we retain the old proc entries and only point them to the new functions?
 
> > 
> > I don't have a solution for the case of a view storing the OID, but Álvaro
> > Herrera suggested to me to at least try preventing those OIDs from being
> > reused.
> > 
> > I've attached a v3 patch set that introduces src/include/catalog/pg_retired.dat
> > to store previously used OIDs and procedure names that the scripts unused_oids
> > and renumber_oids.pl can consume to prevent the reuse of retired OIDs.
> > 
> > Maybe that can also be used towards finding that particular solution...
> I am not sure what can be done, breaking people's databases on pg_upgrade is
> certainly not nice and detecting that function oid has been used anywhere in
> a database sounds painful, especially since there are no references to
> system oids in pg_depend, right?
> 
> That said this patch should be updated to use the new support for default
> values in BKI files.[1]
> 
> https://git.postgresql.org/cgit/postgresql.git/commit/?id=759b03b24ce96f0ba6d734b570d1a6f4a0fb1177

I have attached a new set of patches.  v4 is now using the new support for
default values.

Summary of additional changes:

* I've removed the retired OID tracking, but can certainly add that back if we
  decide it will be useful
* Caught a bug where I wasn't using BoolGetDatum() with DirectFunctionCall3
  with pg_get_expr()


Regards,
Mark
-- 
Mark Wong <markwkm@gmail.com>
EDB https://enterprisedb.com