updates for handling optional argument in system functions

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]https://cirrus-ci.com/task/6109065824174080 -- Mark Wong <markwkm@gmail.com> EDB https://enterprisedb.com 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
--
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 is good. In the other side, I doubt if we should delete proc entries from pg_proc.c? Say, there is a view that uses a proc to 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 is good. In the other side, I doubt if we should delete proc entries from pg_proc.c? Say, there is a view that uses a proc to 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, which is good. In the other side, I doubt if we should delete proc entries from pg_proc.c? Say, there is a view that uses a proc to 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...

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, which is good. In the other side, I doubt if we should delete proc entries from pg_proc.c? Say, there is a view that uses a proc to 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...

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

On 2026-Jan-29, Mark Wong wrote:

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.

Thinking about this again, I wonder where did we get the idea that
reusing OIDs would be a problem. How exactly would this happen? When
you pg_upgrade, your views are taken from a `pg_dump --binary-upgrade`
of the original server, and then recreated using the text
representation of the DDL. We don't pass the function OIDs in any way
from the old server to the new server. And there's no other way (than
pg_upgrade) to go from one major version to the next one where the OID
has been reused.

So why did we think this was an actual problem?

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

On Tue, Mar 03, 2026 at 05:23:58PM +0100, Álvaro Herrera wrote:

On 2026-Jan-29, Mark Wong wrote:

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.

Thinking about this again, I wonder where did we get the idea that
reusing OIDs would be a problem. How exactly would this happen? When
you pg_upgrade, your views are taken from a `pg_dump --binary-upgrade`
of the original server, and then recreated using the text
representation of the DDL. We don't pass the function OIDs in any way
from the old server to the new server. And there's no other way (than
pg_upgrade) to go from one major version to the next one where the OID
has been reused.

So why did we think this was an actual problem?

I'm not sure. I think I see the OID of a function get used in some places
likes rewriteDefine.c and parse_funcs.c, but I'm not sure if I see a function
OID get written out and re-read for a function name lookup in an upgrade code
path, yet...

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

Hi everyone,

I've attached v5, simply a needed rebase due to some other churn in
pg_proc.dat.

Regards,
Mark

On 4/2/26 8:36 PM, Mark Wong wrote:

I've attached v5, simply a needed rebase due to some other churn in
pg_proc.dat.

Nice, I like the patch. The code changes looks good and I like the
removal of these duplicate functions and use of default arguments. But I
think the names of the arguments should be aligned with the names we
have in the documentation. And that may mean that we should change the
documentation.

For example:

= Code

pg_get_ruledef(rule, pretty)

= Docs

pg_get_ruledef(rule_oid, pretty_bool)

= Should docs maybe be updated to the following?

pg_get_ruledef(rule oid, pretty bool)

--
Andreas Karlsson
Percona

On 4/8/26 12:26 AM, Andreas Karlsson wrote:

On 4/2/26 8:36 PM, Mark Wong wrote:

I've attached v5, simply a needed rebase due to some other churn in
pg_proc.dat.

Nice, I like the patch. The code changes looks good and I like the
removal of these duplicate functions and use of default arguments. But I
think the names of the arguments should be aligned with the names we
have in the documentation. And that may mean that we should change the
documentation.

Forgot to attach rebased patches.

--
Andreas Karlsson
Percona

On Wed, Apr 08, 2026 at 12:26:05AM +0200, Andreas Karlsson wrote:

On 4/2/26 8:36 PM, Mark Wong wrote:

I've attached v5, simply a needed rebase due to some other churn in
pg_proc.dat.

Nice, I like the patch. The code changes looks good and I like the
removal of these duplicate functions and use of default arguments. But I
think the names of the arguments should be aligned with the names we
have in the documentation. And that may mean that we should change the
documentation.

Thanks!

For example:

= Code

pg_get_ruledef(rule, pretty)

= Docs

pg_get_ruledef(rule_oid, pretty_bool)

= Should docs maybe be updated to the following?

pg_get_ruledef(rule oid, pretty bool)

I agree with the aligning the names, but maybe I was looking in a
different place?

In the doc/src/sgml/func/func-info.sgml (a4f774cf1c7e) I think I see
pg_get_ruledef(rule oid, pretty bool) already:

<function>pg_get_ruledef</function> ( <parameter>rule</parameter> <type>oid</type> <optional>, <parameter>pretty</parameter> <type>boolean</type> </optional> )

I don't see any matches when I grep for rule_oid or pretty_bool...

Regards,
Mark

On Wed, Apr 08, 2026 at 01:28:29AM +0200, Andreas Karlsson wrote:

On 4/8/26 12:26 AM, Andreas Karlsson wrote:

On 4/2/26 8:36 PM, Mark Wong wrote:

I've attached v5, simply a needed rebase due to some other churn in
pg_proc.dat.

Nice, I like the patch. The code changes looks good and I like the
removal of these duplicate functions and use of default arguments. But I
think the names of the arguments should be aligned with the names we
have in the documentation. And that may mean that we should change the
documentation.

Forgot to attach rebased patches.

Another rebase needed so quickly? :) Thanks for the assist!

Regards,
Mark

On 4/8/26 1:42 AM, Mark Wong wrote:

I don't see any matches when I grep for rule_oid or pretty_bool...

Sorry for the noise, I was looking at an old version of the docs. The
parameters do indeed match the docs. But now that I looked at the
patches again I found a real issue which I think should be fixed.

I think the following:

"select statement of a view with pretty-print option"

should likely just be:

"select statement of a view"

Andreas

On Apr 8, 2026, at 07:44, Mark Wong <markwkm@gmail.com> wrote:

On Wed, Apr 08, 2026 at 01:28:29AM +0200, Andreas Karlsson wrote:

On 4/8/26 12:26 AM, Andreas Karlsson wrote:

On 4/2/26 8:36 PM, Mark Wong wrote:

I've attached v5, simply a needed rebase due to some other churn in
pg_proc.dat.

Nice, I like the patch. The code changes looks good and I like the
removal of these duplicate functions and use of default arguments. But I
think the names of the arguments should be aligned with the names we
have in the documentation. And that may mean that we should change the
documentation.

Forgot to attach rebased patches.

Another rebase needed so quickly? :) Thanks for the assist!

Regards,
Mark

I still have the question as I raised previously. Let’s use 0001 as an example. 0001 removes function 1573.

Say, an existing view depends on 1573:
```
evantest=# CREATE VIEW v_ruledef AS
evantest-# SELECT pg_get_ruledef(oid) AS ruledef
evantest-# FROM pg_rewrite
evantest-# WHERE rulename = '_RETURN';
CREATE VIEW
```

The view is stored as:
```
_RETURN | v_ruledef | ({QUERY :commandType 1 :querySource 0 :canSetTag true :utilityStmt <> :resultRelation 0 :forPortionOf <> :hasAggs false :hasWindowFuncs false :hasTargetSRFs false :hasSubLinks false :hasDistinctOn false :hasRecursive false :hasModifyingCTE false :hasForUpdate false :hasRowSecurity false :hasGroupRTE false :isReturn false :cteList <> :rtable ({RANGETBLENTRY :alias <> :eref {ALIAS :aliasname pg_rewrite :colnames ("oid" "rulename" "ev_class" "ev_type" "ev_enabled" "is_instead" "ev_qual" "ev_action")} :rtekind 0 :relid 2618 :inh true :relkind r :rellockmode 1 :perminfoindex 1 :tablesample <> :lateral false :inFromCl true :securityQuals <>}) :rteperminfos ({RTEPERMISSIONINFO :relid 2618 :inh true :requiredPerms 2 :checkAsUser 0 :selectedCols (b 8 9) :insertedCols (b) :updatedCols (b)}) :jointree {FROMEXPR :fromlist ({RANGETBLREF :rtindex 1}) :quals {OPEXPR :opno 93 :opfuncid 62 :opresulttype 16 :opretset false :opcollid 0 :inputcollid 950 :args ({VAR :varno 1 :varattno 2 :vartype 19 :vartypmod -1 :varcollid 950 :varnullingrels (b) :varlevelsup 0 :varreturningtype 0 :varnosyn 1 :varattnosyn 2 :location -1} {CONST :consttype 19 :consttypmod -1 :constcollid 950 :constlen 64 :constbyval false :constisnull false :location -1 :constvalue 64 [ 95 82 69 84 85 82 78 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 ]}) :location -1}} :mergeActionList <> :mergeTargetRelation 0 :mergeJoinCondition <> :targetList ({TARGETENTRY :expr {FUNCEXPR :funcid 1573 :funcresulttype 25 :funcretset false :funcvariadic false :funcformat 0 :funccollid 100 :inputcollid 0 :args ({VAR :varno 1 :varattno 1 :vartype 26 :vartypmod -1 :varcollid 0 :varnullingrels (b) :varlevelsup 0 :varreturningtype 0 :varnosyn 1 :varattnosyn 1 :location -1}) :location -1} :resno 1 :resname ruledef :ressortgroupref 0 :resorigtbl 0 :resorigcol 0 :resjunk false}) :override 0 :onConflict <> :returningOldAlias <> :returningNewAlias <> :returningList <> :groupClause <> :groupDistinct false :groupByAll false :groupingSets <> :havingQual <> :windowClause <> :distinctClause <> :sortClause <> :limitOffset <> :limitCount <> :limitOption 0 :rowMarks <> :setOperations <> :constraintDeps <> :withCheckOptions <> :stmt_location -1 :stmt_len -1})
```

We can clearly see ":expr {FUNCEXPR :funcid 1573 “.

With this patch, will that view break? How would users find all such broken views? Maybe PostgreSQL already has some recommended way to handle this kind of situation that I am not aware of?

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

On Tuesday, April 7, 2026, Chao Li <li.evan.chao@gmail.com> wrote:

We can clearly see ":expr {FUNCEXPR :funcid 1573 “.

With this patch, will that view break? How would users find all such
broken views? Maybe PostgreSQL already has some recommended way to handle
this kind of situation that I am not aware of?

pg_dump resolves oid=1573 and produces a textual SQL representation, which
is then executed during pg_restore. This happens manually, and also
automatically by pg_upgrade. Since the text form hasn’t changed the view
is still valid in v19. You would see the new oid if inspecting the rule
after the upgrade.

So yes, the public serialization format being SQL and thus mandatory new
object creation during upgrade is the way PostgreSQL handles implementation
detail isolation.

David J.

On Apr 8, 2026, at 09:46, David G. Johnston <david.g.johnston@gmail.com> wrote:

On Tuesday, April 7, 2026, Chao Li <li.evan.chao@gmail.com> wrote:

We can clearly see ":expr {FUNCEXPR :funcid 1573 “.

With this patch, will that view break? How would users find all such broken views? Maybe PostgreSQL already has some recommended way to handle this kind of situation that I am not aware of?

pg_dump resolves oid=1573 and produces a textual SQL representation, which is then executed during pg_restore. This happens manually, and also automatically by pg_upgrade. Since the text form hasn’t changed the view is still valid in v19. You would see the new oid if inspecting the rule after the upgrade.

So yes, the public serialization format being SQL and thus mandatory new object creation during upgrade is the way PostgreSQL handles implementation detail isolation.

David J.

Hi David, thanks for the explanation, I really didn’t know that.

Then, the patch looks good to me. GET_PRETTY_FLAGS(false) returns PRETTYFLAG_INDENT, so no behavior change either.

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

On Wed, Apr 08, 2026 at 01:51:30AM +0200, Andreas Karlsson wrote:

On 4/8/26 1:42 AM, Mark Wong wrote:

I don't see any matches when I grep for rule_oid or pretty_bool...

Sorry for the noise, I was looking at an old version of the docs. The
parameters do indeed match the docs. But now that I looked at the
patches again I found a real issue which I think should be fixed.

No worries.

I think the following:

"select statement of a view with pretty-print option"

should likely just be:

"select statement of a view"

Yeah, I agree with that considering that other functions like the recent
get_*_ddl functions have options like that aren't detailed in
pg_proc.dat.

What I pondered over a little bit was whether to flip-flop and remove
the original _ext definitions instead and modify their respective
counterparts that I originally removed. I opted to continue editing what
I started because of the comment that reads "System-view support
functions with pretty-print option", but I don't have any strong
opinions either way.

I've attached v7 with the more succinct descriptions.

Regards,
Mark

On 4/8/26 8:10 PM, Mark Wong wrote:

What I pondered over a little bit was whether to flip-flop and remove
the original _ext definitions instead and modify their respective
counterparts that I originally removed. I opted to continue editing what
I started because of the comment that reads "System-view support
functions with pretty-print option", but I don't have any strong
opinions either way.

I've attached v7 with the more succinct descriptions.

I would be fine with either too. The comment feels a bit less useful now
so it too could either stay or go. I have no strong feelings about it.

Setting this as ready for committer since I think the patch is fine
as-is and an improvement over the current no matter what is done about
the comment or which oids we decide to use.

Andreas