IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

Started by jian heabout 1 month ago22 messageshackers
Jump to latest
#1jian he
jian.universality@gmail.com

Hi.

While reviewing the row pattern recognition,
I noticed that the IGNORE NULLS and RESPECT NULLS clauses can be specified for
normal functions (prokind == 'f').

create or replace function dummy2(int) returns bigint AS $$ BEGIN
RETURN $1; END$$ STABLE LANGUAGE plpgsql;
select dummy2(1) IGNORE NULLS;
select dummy2(1) RESPECT NULLS;

This seems unexpected?

Also https://git.postgresql.org/cgit/postgresql.git/commit/?id=25a30bbd4235a49c854036c84fe90f2bc5a87652
Obviously, RESPECT NULLS and IGNORE NULLS can appear in SELECT query,
since they are dedicated keywords, shouldn't they be mentioned in
select.sgml?

--
jian
https://www.enterprisedb.com/

#2Tatsuo Ishii
ishii@postgresql.org
In reply to: jian he (#1)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

While reviewing the row pattern recognition,
I noticed that the IGNORE NULLS and RESPECT NULLS clauses can be specified for
normal functions (prokind == 'f').

create or replace function dummy2(int) returns bigint AS $$ BEGIN
RETURN $1; END$$ STABLE LANGUAGE plpgsql;
select dummy2(1) IGNORE NULLS;
select dummy2(1) RESPECT NULLS;

This seems unexpected?

Yes. Other than true window functions are not allowed to use the null
treatment clause per spec.

Also https://git.postgresql.org/cgit/postgresql.git/commit/?id=25a30bbd4235a49c854036c84fe90f2bc5a87652
Obviously, RESPECT NULLS and IGNORE NULLS can appear in SELECT query,
since they are dedicated keywords, shouldn't they be mentioned in
select.sgml?

No, we should fix the code. See attached patch.

To fix this, ParseFuncOrColumn() now checks whether other than window
functions are used with the null treatment clause. If so, error out.

Also remove the unnecessary test for "aggregate functions do not
accept RESPECT/IGNORE NULLS" because it is now checked in the
early-stage new check. The window regression test expected file is
changed accordingly.

Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

Attachments:

v1-0001-Fix-to-not-allow-null-treatment-to-non-window-fun.patchtext/x-patch; charset=us-asciiDownload+11-10
#3Tatsuo Ishii
ishii@postgresql.org
In reply to: Tatsuo Ishii (#2)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

While reviewing the row pattern recognition,
I noticed that the IGNORE NULLS and RESPECT NULLS clauses can be specified for
normal functions (prokind == 'f').

create or replace function dummy2(int) returns bigint AS $$ BEGIN
RETURN $1; END$$ STABLE LANGUAGE plpgsql;
select dummy2(1) IGNORE NULLS;
select dummy2(1) RESPECT NULLS;

This seems unexpected?

Yes. Other than true window functions are not allowed to use the null
treatment clause per spec.

Also https://git.postgresql.org/cgit/postgresql.git/commit/?id=25a30bbd4235a49c854036c84fe90f2bc5a87652
Obviously, RESPECT NULLS and IGNORE NULLS can appear in SELECT query,
since they are dedicated keywords, shouldn't they be mentioned in
select.sgml?

No, we should fix the code. See attached patch.

To fix this, ParseFuncOrColumn() now checks whether other than window
functions are used with the null treatment clause. If so, error out.

Also remove the unnecessary test for "aggregate functions do not
accept RESPECT/IGNORE NULLS" because it is now checked in the
early-stage new check. The window regression test expected file is
changed accordingly.

I'm going to push the patch today if there's no objection.
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

#4jian he
jian.universality@gmail.com
In reply to: Tatsuo Ishii (#3)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

On Wed, Jun 17, 2026 at 7:47 AM Tatsuo Ishii <ishii@postgresql.org> wrote:

To fix this, ParseFuncOrColumn() now checks whether other than window
functions are used with the null treatment clause. If so, error out.

Also remove the unnecessary test for "aggregate functions do not
accept RESPECT/IGNORE NULLS" because it is now checked in the
early-stage new check. The window regression test expected file is
changed accordingly.

I'm going to push the patch today if there's no objection.

The code looks good to me, and the commit message is clear and well-written.

#5Tatsuo Ishii
ishii@postgresql.org
In reply to: jian he (#4)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

To fix this, ParseFuncOrColumn() now checks whether other than window
functions are used with the null treatment clause. If so, error out.

Also remove the unnecessary test for "aggregate functions do not
accept RESPECT/IGNORE NULLS" because it is now checked in the
early-stage new check. The window regression test expected file is
changed accordingly.

I'm going to push the patch today if there's no objection.

The code looks good to me, and the commit message is clear and well-written.

Thank you for the review!
I have pushed the patch to master branch.
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4e5920e6de87dc1ed761d2a898b7786c50c11db2
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

#6Chao Li
li.evan.chao@gmail.com
In reply to: Tatsuo Ishii (#5)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

On Jun 17, 2026, at 09:17, Tatsuo Ishii <ishii@postgresql.org> wrote:

To fix this, ParseFuncOrColumn() now checks whether other than window
functions are used with the null treatment clause. If so, error out.

Also remove the unnecessary test for "aggregate functions do not
accept RESPECT/IGNORE NULLS" because it is now checked in the
early-stage new check. The window regression test expected file is
changed accordingly.

I'm going to push the patch today if there's no objection.

The code looks good to me, and the commit message is clear and well-written.

Thank you for the review!
I have pushed the patch to master branch.
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4e5920e6de87dc1ed761d2a898b7786c50c11db2
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

Hi Tatsuo-san,

Sorry for jumping in. I didn’t review the patch, but while browsing recent commits, I noticed that this commit has a small issue: the errmsg should start with a lowercase character, according to the error style guide [1]https://www.postgresql.org/docs/current/error-style-guide.html:
```
Primary error messages: Do not capitalize the first letter. Do not end a message with a period. Do not even think about ending a message with an exclamation point.
```

The attached diff is a quick fix that changes “Only” to “only” and updates the expected output accordingly.

[1]: https://www.postgresql.org/docs/current/error-style-guide.html

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

Attachments:

nocfbot-fix-errmsg.diffapplication/octet-stream; name=nocfbot-fix-errmsg.diff; x-unix-mode=0644Download+3-3
#7Tatsuo Ishii
ishii@postgresql.org
In reply to: Chao Li (#6)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

Hi Chao,

Hi Tatsuo-san,

Sorry for jumping in. I didn’t review the patch, but while browsing recent commits, I noticed that this commit has a small issue: the errmsg should start with a lowercase character, according to the error style guide [1]:
```
Primary error messages: Do not capitalize the first letter. Do not end a message with a period. Do not even think about ending a message with an exclamation point.
```

The attached diff is a quick fix that changes “Only” to “only” and updates the expected output accordingly.

[1] https://www.postgresql.org/docs/current/error-style-guide.html

You are right. I don't know why I forgot the rule. Patch pushed.
Thank you!
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

#8Fujii Masao
masao.fujii@gmail.com
In reply to: Tatsuo Ishii (#7)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

On Wed, Jun 17, 2026 at 12:58 PM Tatsuo Ishii <ishii@postgresql.org> wrote:

Hi Chao,

Hi Tatsuo-san,

Sorry for jumping in. I didn’t review the patch, but while browsing recent commits, I noticed that this commit has a small issue: the errmsg should start with a lowercase character, according to the error style guide [1]:
```
Primary error messages: Do not capitalize the first letter. Do not end a message with a period. Do not even think about ending a message with an exclamation point.
```

The attached diff is a quick fix that changes “Only” to “only” and updates the expected output accordingly.

[1] https://www.postgresql.org/docs/current/error-style-guide.html

You are right. I don't know why I forgot the rule. Patch pushed.
Thank you!

Thanks for working on this!

I have a couple of comments.

When executing a non-existent function with IGNORE NULLS, for example,

SELECT no_such_func() IGNORE NULLS;

previously we got:

ERROR: function no_such_func() does not exist

but now we get:

ERROR: only window functions accept RESPECT/IGNORE NULLS

Isn't the previous error more helpful in this case? It makes me wonder
whether the IGNORE NULLS check is being performed too early. Perhaps
it would be better to move the check to other place.

I also noticed that when using an aggregate function as a window
function, for example,

SELECT sum(i) IGNORE NULLS OVER() FROM ...;

we now get:

ERROR: only window functions accept RESPECT/IGNORE NULLS

whereas previously the error was:

ERROR: aggregate functions do not accept RESPECT/IGNORE NULLS

The new message seems confusing because `sum()` is being used as
a window function in this case, so saying "only window functions accept ..."
doesn't seem accurate.

/*
* NULL TREATEMENT is only allowed for window functions per spec.
*/

Also, I noticed a typo: TREATEMENT should be TREATMENT.

The attached patch implements these changes. Thoughts?

Regards,

--
Fujii Masao

Attachments:

v1-0001-Refine-NULL-treatment-checks-for-non-window-funct.patchapplication/octet-stream; name=v1-0001-Refine-NULL-treatment-checks-for-non-window-funct.patchDownload+33-12
#9Tatsuo Ishii
ishii@postgresql.org
In reply to: Fujii Masao (#8)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

Hi,

Thanks for working on this!

I have a couple of comments.

When executing a non-existent function with IGNORE NULLS, for example,

SELECT no_such_func() IGNORE NULLS;

previously we got:

ERROR: function no_such_func() does not exist

but now we get:

ERROR: only window functions accept RESPECT/IGNORE NULLS

Isn't the previous error more helpful in this case? It makes me wonder
whether the IGNORE NULLS check is being performed too early. Perhaps
it would be better to move the check to other place.

Agreed. BTW, if the check is moved there, it might be better to
change the error message according to the surroundings. The pattern is
"... specified, but %s is not a ..."

errmsg("OVER specified, but %s is not a window function nor an aggregate function",

So the message could be changed to something like:

errmsg("RESPECT/IGNORE NULLS is specified, but %s is not a window function"),

I also noticed that when using an aggregate function as a window
function, for example,

SELECT sum(i) IGNORE NULLS OVER() FROM ...;

we now get:

ERROR: only window functions accept RESPECT/IGNORE NULLS

whereas previously the error was:

ERROR: aggregate functions do not accept RESPECT/IGNORE NULLS

The new message seems confusing because `sum()` is being used as
a window function in this case, so saying "only window functions accept ..."
doesn't seem accurate.

I agree with your concern and am fine with the change.

/*
* NULL TREATEMENT is only allowed for window functions per spec.
*/

Also, I noticed a typo: TREATEMENT should be TREATMENT.

Thanks for the correction.

The attached patch implements these changes. Thoughts?

Please see comment above.

Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

#10Chao Li
li.evan.chao@gmail.com
In reply to: Tatsuo Ishii (#9)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

On Jun 18, 2026, at 10:44, Tatsuo Ishii <ishii@postgresql.org> wrote:

Hi,

Thanks for working on this!

I have a couple of comments.

When executing a non-existent function with IGNORE NULLS, for example,

SELECT no_such_func() IGNORE NULLS;

previously we got:

ERROR: function no_such_func() does not exist

but now we get:

ERROR: only window functions accept RESPECT/IGNORE NULLS

Isn't the previous error more helpful in this case? It makes me wonder
whether the IGNORE NULLS check is being performed too early. Perhaps
it would be better to move the check to other place.

Agreed. BTW, if the check is moved there, it might be better to
change the error message according to the surroundings. The pattern is
"... specified, but %s is not a ..."

errmsg("OVER specified, but %s is not a window function nor an aggregate function",

So the message could be changed to something like:

errmsg("RESPECT/IGNORE NULLS is specified, but %s is not a window function"),

Yes, this is also my only comment for the patch.

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

#11Fujii Masao
masao.fujii@gmail.com
In reply to: Chao Li (#10)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

On Thu, Jun 18, 2026 at 12:06 PM Chao Li <li.evan.chao@gmail.com> wrote:

Agreed. BTW, if the check is moved there, it might be better to
change the error message according to the surroundings. The pattern is
"... specified, but %s is not a ..."

errmsg("OVER specified, but %s is not a window function nor an aggregate function",

So the message could be changed to something like:

errmsg("RESPECT/IGNORE NULLS is specified, but %s is not a window function"),

Thanks for the review!

I've updated the patch accordingly.

I removed "is" from "RESPECT/IGNORE NULLS is specified" to match other
similar error messages, which use the form "XXX specified, ...".

+ if (ignore_nulls != NO_NULLTREATMENT)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("aggregate functions do not accept RESPECT/IGNORE NULLS"),
+ parser_errposition(pstate, location)));

I also moved this check to the end of the
"else if (fdresult == FUNCDETAIL_AGGREGATE)" block. That seems like
a better place, since the ordered-set aggregate checks should happen
before this check.

Regards,

--
Fujii Masao

Attachments:

v2-0001-Refine-error-reporting-for-null-treatment-on-non-.patchapplication/octet-stream; name=v2-0001-Refine-error-reporting-for-null-treatment-on-non-.patchDownload+31-12
#12Chao Li
li.evan.chao@gmail.com
In reply to: Fujii Masao (#11)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

On Jun 18, 2026, at 15:54, Fujii Masao <masao.fujii@gmail.com> wrote:

On Thu, Jun 18, 2026 at 12:06 PM Chao Li <li.evan.chao@gmail.com> wrote:

Agreed. BTW, if the check is moved there, it might be better to
change the error message according to the surroundings. The pattern is
"... specified, but %s is not a ..."

errmsg("OVER specified, but %s is not a window function nor an aggregate function",

So the message could be changed to something like:

errmsg("RESPECT/IGNORE NULLS is specified, but %s is not a window function"),

Thanks for the review!

I've updated the patch accordingly.

I removed "is" from "RESPECT/IGNORE NULLS is specified" to match other
similar error messages, which use the form "XXX specified, ...".

+ if (ignore_nulls != NO_NULLTREATMENT)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("aggregate functions do not accept RESPECT/IGNORE NULLS"),
+ parser_errposition(pstate, location)));

I also moved this check to the end of the
"else if (fdresult == FUNCDETAIL_AGGREGATE)" block. That seems like
a better place, since the ordered-set aggregate checks should happen
before this check.

Regards,

--
Fujii Masao
<v2-0001-Refine-error-reporting-for-null-treatment-on-non-.patch>

V2 looks good to me.

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

#13Tatsuo Ishii
ishii@postgresql.org
In reply to: Fujii Masao (#11)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

I've updated the patch accordingly.

I removed "is" from "RESPECT/IGNORE NULLS is specified" to match other
similar error messages, which use the form "XXX specified, ...".

+ if (ignore_nulls != NO_NULLTREATMENT)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),

The errcode field can start without a parenthesis:

errcode(ERRCODE_WRONG_OBJECT_TYPE),

Other than that the patch looks good to me.

Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

#14Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Fujii Masao (#11)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

Hello,

I would dearly appreciate it if you could make these errors in this
style:

ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
/* translator: first %s is a CREATE FUNCTION clause, eg. "IGNORE NULLS" */
errmsg("%s specified, but %s is not a window function",
"RESPECT/IGNORE NULLS", NameListToString(funcname)),
parser_errposition(pstate, location)));

to avoid keeping the clause name in the translatable string.

Thanks

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Entristecido, Wutra (canción de Las Barreras)
echa a Freyr a rodar
y a nosotros al mar"

#15Fujii Masao
masao.fujii@gmail.com
In reply to: Alvaro Herrera (#14)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

On Thu, Jun 18, 2026 at 11:46 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

Hello,

I would dearly appreciate it if you could make these errors in this
style:

ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
/* translator: first %s is a CREATE FUNCTION clause, eg. "IGNORE NULLS" */
errmsg("%s specified, but %s is not a window function",
"RESPECT/IGNORE NULLS", NameListToString(funcname)),
parser_errposition(pstate, location)));

to avoid keeping the clause name in the translatable string.

Thanks for the suggestion! I've updated the patch accordingly.
Patch attached.

Regards,

--
Fujii Masao

Attachments:

v3-0001-Refine-error-reporting-for-null-treatment-on-non-.patchapplication/octet-stream; name=v3-0001-Refine-error-reporting-for-null-treatment-on-non-.patchDownload+32-12
#16Fujii Masao
masao.fujii@gmail.com
In reply to: Tatsuo Ishii (#13)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

On Thu, Jun 18, 2026 at 5:28 PM Tatsuo Ishii <ishii@postgresql.org> wrote:

I've updated the patch accordingly.

I removed "is" from "RESPECT/IGNORE NULLS is specified" to match other
similar error messages, which use the form "XXX specified, ...".

+ if (ignore_nulls != NO_NULLTREATMENT)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),

The errcode field can start without a parenthesis:

errcode(ERRCODE_WRONG_OBJECT_TYPE),

Yes, but other similar ereport() calls in parse_func.c also start
the errcode field with a parenthesis, so I followed the same style
here.

Regards,

--
Fujii Masao

#17Fujii Masao
masao.fujii@gmail.com
In reply to: Fujii Masao (#15)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

On Fri, Jun 19, 2026 at 1:12 AM Fujii Masao <masao.fujii@gmail.com> wrote:

On Thu, Jun 18, 2026 at 11:46 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

Hello,

I would dearly appreciate it if you could make these errors in this
style:

ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
/* translator: first %s is a CREATE FUNCTION clause, eg. "IGNORE NULLS" */
errmsg("%s specified, but %s is not a window function",
"RESPECT/IGNORE NULLS", NameListToString(funcname)),
parser_errposition(pstate, location)));

to avoid keeping the clause name in the translatable string.

Thanks for the suggestion! I've updated the patch accordingly.
Patch attached.

Barring any objections, I will commit the patch.

Regards,

--
Fujii Masao

#18Tatsuo Ishii
ishii@postgresql.org
In reply to: Fujii Masao (#17)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

On Fri, Jun 19, 2026 at 1:12 AM Fujii Masao <masao.fujii@gmail.com> wrote:

On Thu, Jun 18, 2026 at 11:46 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

Hello,

I would dearly appreciate it if you could make these errors in this
style:

ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
/* translator: first %s is a CREATE FUNCTION clause, eg. "IGNORE NULLS" */
errmsg("%s specified, but %s is not a window function",
"RESPECT/IGNORE NULLS", NameListToString(funcname)),
parser_errposition(pstate, location)));

to avoid keeping the clause name in the translatable string.

Thanks for the suggestion! I've updated the patch accordingly.
Patch attached.

Barring any objections, I will commit the patch.

No objection from me.
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

#19Fujii Masao
masao.fujii@gmail.com
In reply to: Tatsuo Ishii (#18)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

On Tue, Jun 23, 2026 at 8:38 PM Tatsuo Ishii <ishii@postgresql.org> wrote:

Barring any objections, I will commit the patch.

No objection from me.

I've pushed the patch. Thanks!

Regards,

--
Fujii Masao

#20Tatsuo Ishii
ishii@postgresql.org
In reply to: Fujii Masao (#19)
Re: IGNORE/RESPECT NULLS can be specified for (prokind == 'f').

On Tue, Jun 23, 2026 at 8:38 PM Tatsuo Ishii <ishii@postgresql.org> wrote:

Barring any objections, I will commit the patch.

No objection from me.

I've pushed the patch. Thanks!

Thank you so much!
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

#21Henson Choi
assam258@gmail.com
In reply to: Fujii Masao (#19)
#22Fujii Masao
masao.fujii@gmail.com
In reply to: Henson Choi (#21)