From: Nikolay Shaplov <dhyan@nataraj.su>
To: pgsql-hackers@postgresql.org, pgsql-hackers@lists.postgresql.org
Cc: 
 =?UTF-8?B?0JDQu9C10L3QsCDQktCw0YHQuNC70YzQtdCy0LA=?= <gorcom2012@gmail.com>
Subject: Re: PATCH for BUG #18785: Pointer bmr.rel,
 dereferenced by passing as 1st parameter to function is checked for NULL
 later
Date: Mon, 20 Oct 2025 14:14:29 +0300
Message-ID: <5610605.Sb9uPGUboI@thinkpad-pgpro>
Organization: Postgres Professional
In-Reply-To: 
 <CABg3sZqO_TVXUcH4NvOqeRFYFb+QzEdQDpKiO_zp=AsJi2QJPw@mail.gmail.com>
References: 
 <64c6fbba20444c759eb7f9ebdc386c36@mt-integration.ru>
 <CABg3sZqO_TVXUcH4NvOqeRFYFb+QzEdQDpKiO_zp=AsJi2QJPw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart2028875.yKVeVyVuyW";
 micalg="pgp-sha512"; protocol="application/pgp-signature"
Archived-At: 
 <https://www.postgresql.org/message-id/5610605.Sb9uPGUboI%40thinkpad-pgpro>
Precedence: bulk

--nextPart2028875.yKVeVyVuyW
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"; protected-headers="v1"
From: Nikolay Shaplov <dhyan@nataraj.su>
To: pgsql-hackers@postgresql.org, pgsql-hackers@lists.postgresql.org
Date: Mon, 20 Oct 2025 14:14:29 +0300
Message-ID: <5610605.Sb9uPGUboI@thinkpad-pgpro>
Organization: Postgres Professional
MIME-Version: 1.0

=D0=92 =D0=BF=D0=B8=D1=81=D1=8C=D0=BC=D0=B5 =D0=BE=D1=82 =D1=87=D0=B5=D1=82=
=D0=B2=D0=B5=D1=80=D0=B3, 18 =D1=81=D0=B5=D0=BD=D1=82=D1=8F=D0=B1=D1=80=D1=
=8F 2025=E2=80=AF=D0=B3. 11:44:57 =D0=9C=D0=BE=D1=81=D0=BA=D0=B2=D0=B0, =D1=
=81=D1=82=D0=B0=D0=BD=D0=B4=D0=B0=D1=80=D1=82=D0=BD=D0=BE=D0=B5 =D0=B2=D1=
=80=D0=B5=D0=BC=D1=8F=20
=D0=BF=D0=BE=D0=BB=D1=8C=D0=B7=D0=BE=D0=B2=D0=B0=D1=82=D0=B5=D0=BB=D1=8C =
=D0=90=D0=BB=D0=B5=D0=BD=D0=B0 =D0=92=D0=B0=D1=81=D0=B8=D0=BB=D1=8C=D0=B5=
=D0=B2=D0=B0 =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB:

> I propose adding an Assert at this point as a way to document the contract
> between this code and its environment.
> This was previously discussed in BUG #18785 in pqsql-bugs
> https://www.postgresql.org/message-id/flat/a6oxxee6blexicuark46yydtaqulsj=
vkr
> wkri6aic4vbofjxse%404a6j4kuwda7u#c52de413b182d9c42fe1eb34a82871b5

Hi!

I'd like to join reviewing process. I am already familiar with this issue f=
rom=20
previous discussions.

PROS:=20
This issue as I know have been found while applying static analyzer to=20
PostgreSQL code. This is not a bug per ce, everything works as expected, th=
ere=20
is certain calling convention that makes dubious situation unreachable.

But what static analyzer is actually reporting here: there is some calling=
=20
convention that have not been explicitly declared, and this might lead to=20
problems in future: somebody breaks this convention, but we will never know=
=20
about it.=20

=46rom my point of view, the best way to declare calling convention in=20
PostgreSQL are Asserts + human readable comments explaining these Asserts.

This will stop static analyzer from raising false alarm, and these Asserts=
=20
will raise a true alarm if some developer tries to break that convention. A=
nd=20
comments near the Assert will hopefully help him understanding what is wron=
g=20
here.

So it is win-win situation: we improve code quality, and future users of=20
static analyzers are no longer bored with false alarms.

So in general this patch should be acceppted

CONS:
But this patch application should be redo from scratch.

1. If you want to submit patch, add it to the commit fest. Not only post it=
 on=20
the mailing list.

2. Patch should be attach to a letter, not embedded in message text.=20
Commitfest robot will not be able to extract it from there

3. To prepare patch you should use `git format-patch` command. And please a=
dd=20
please create a prototype of a commit message that commiter will use to=20
creating final one, he will use while committing this patch. Do not make=20
committer invent it from scratch :-)

You can find more guides for patch submitting via these links:
https://wiki.postgresql.org/wiki/Submitting_a_Patch

https://peter.eisentraut.org/blog/2023/05/09/how-to-submit-a-patch-by-email=
=2D2023-edition

4.

> +   /*
> +    * Validate the contract between flags and bmr.rel.
> +    *

This "validate" word might accidentally confuse someone. Because Asserts in=
=20
postgres clearly do no validation, they are completely removed from code in=
=20
production builds, so no checks are done. Asserts are more a way of setting=
=20
traps for future call convention violator,s that will be triggered while=20
running tests, fuzzing or any other non-production activities that includes=
=20
turning Asserts on. So "valudate" is a bad word here....


So please submit patch accordingly and I will be a reviewer for it.


=2D-=20
Nikolay Shaplov aka Nataraj
=46uzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

--nextPart2028875.yKVeVyVuyW
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part.
Content-Transfer-Encoding: 7Bit

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEE+sk3ebqQKlezKOi8PMbfuIHAGpgFAmj2GZUACgkQPMbfuIHA
Gpg4nwf/WGD9rGZJvQhulcAkH9E6XH9h2qKNgF56jWqREPMHCqQb7BRRYGD6bDzR
zqjGpTPOlzsi3z4Ty5Q4AFNfjq37JpmX0JBP69n6GPbWVXaccy+yoDQ7AbmaNOWH
63e2WpTsVjh6NpLFVLxURUZRqAqbL6YXCNjriNNaeSokSh//zAcjyyeSMYe4R1Re
Xu0QPjRX56dhS1RssrjOggXRJSOUYWHhWbV+JCC3LxG1ulk05/yruu0U0Evj/0P3
vrb7Nj5ipAzKk28jxnZB88H5RMNR8QaCmZIiWyQ+RDpTEMmrUh8gvUavbSpZsa/9
nYELpdeiHXLgWjTmlcmbQuK77q3+1g==
=GFl9
-----END PGP SIGNATURE-----

--nextPart2028875.yKVeVyVuyW--