Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Mark function arguments of type "Datum *" as "const Datum *" where possible

  1. Mark function arguments of type "Datum *" as "const Datum *" where possible

    Chao Li <li.evan.chao@gmail.com> — 2025-09-26T05:56:30Z

    Hi Hackers,

I noticed that many functions take "Datum *" parameters while they don't
update the data. So I created this patch to change "Datum *" to "const
Datum *" wherever possible, which should improve type safety and make the
interfaces clearer about their intent, also helps the compiler catch
accidental modifications.

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

  2. Re: Mark function arguments of type "Datum *" as "const Datum *" where possible

    Chao Li <li.evan.chao@gmail.com> — 2025-09-26T07:35:54Z

    Revision to v2.

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


On Fri, Sep 26, 2025 at 1:56 PM Chao Li <li.evan.chao@gmail.com> wrote:

> Hi Hackers,
>
> I noticed that many functions take "Datum *" parameters while they don't
> update the data. So I created this patch to change "Datum *" to "const
> Datum *" wherever possible, which should improve type safety and make the
> interfaces clearer about their intent, also helps the compiler catch
> accidental modifications.
>
> Best regards,
> Chao Li
> ---------------------
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>

  3. Re: Mark function arguments of type "Datum *" as "const Datum *" where possible

    Chao Li <li.evan.chao@gmail.com> — 2025-09-26T09:51:13Z

    
> On Sep 26, 2025, at 15:35, Chao Li <li.evan.chao@gmail.com> wrote:
> 
> Revision to v2.
> 
> 
> <v2-0001-Mark-function-arguments-of-type-Datum-as-const-Da.patch>

CF Link: https://commitfest.postgresql.org/patch/6081/

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

  4. Re: Mark function arguments of type "Datum *" as "const Datum *" where possible

    Michael Paquier <michael@paquier.xyz> — 2025-09-26T11:50:10Z

    On Fri, Sep 26, 2025 at 03:35:54PM +0800, Chao Li wrote:
>> I noticed that many functions take "Datum *" parameters while they don't
>> update the data. So I created this patch to change "Datum *" to "const
>> Datum *" wherever possible, which should improve type safety and make the
>> interfaces clearer about their intent, also helps the compiler catch
>> accidental modifications.

I have not looked at the patch in details and we need a careful
case-by-case review, but being more protective with the Datums that
travel across the stack may be a good idea in the long-term depending
on the code path dealt with, so I like the initiative you are taking
here.
--
Michael

  5. Re: Mark function arguments of type "Datum *" as "const Datum *" where possible

    Nathan Bossart <nathandbossart@gmail.com> — 2025-09-26T14:30:54Z

    On Fri, Sep 26, 2025 at 08:50:10PM +0900, Michael Paquier wrote:
> On Fri, Sep 26, 2025 at 03:35:54PM +0800, Chao Li wrote:
>>> I noticed that many functions take "Datum *" parameters while they don't
>>> update the data. So I created this patch to change "Datum *" to "const
>>> Datum *" wherever possible, which should improve type safety and make the
>>> interfaces clearer about their intent, also helps the compiler catch
>>> accidental modifications.
> 
> I have not looked at the patch in details and we need a careful
> case-by-case review, but being more protective with the Datums that
> travel across the stack may be a good idea in the long-term depending
> on the code path dealt with, so I like the initiative you are taking
> here.

I have mixed feelings about this patch.  I have no concrete objections to
the technical content, but some questions come to mind.  For example, why
are we only fixing Datum parameters?  Are we going to fix other types
later?  Also, I'm a bit worried that this will lead to several rounds of
similar patches, which IMHO is arguably unnecessary churn that could lead
to back-patching pain.  I personally would be more likely to fix these
sorts of things in passing as part of another patch that touches the same
area of code.

That being said, I won't object if Michael or someone else feels it's
worthwhile.  I usually try to add const to new function parameters whenever
possible, for exactly the reasons given as motivation for this patch.

-- 
nathan

  6. Re: Mark function arguments of type "Datum *" as "const Datum *" where possible

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-26T15:00:13Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> I have mixed feelings about this patch.  I have no concrete objections to
> the technical content, but some questions come to mind.  For example, why
> are we only fixing Datum parameters?

Yeah.  In particular, probably 99% of such Datum arrays also have an
associated array of bool isnull flags.  IMO it makes exactly zero
sense to const-ify the Datums without similar protection for their
isnull flags.

			regards, tom lane

  7. Re: Mark function arguments of type "Datum *" as "const Datum *" where possible

    Chao Li <li.evan.chao@gmail.com> — 2025-09-28T03:02:32Z

    On Fri, Sep 26, 2025 at 11:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Yeah.  In particular, probably 99% of such Datum arrays also have an
> associated array of bool isnull flags.  IMO it makes exactly zero
> sense to const-ify the Datums without similar protection for their
> isnull flags.
>
>
Based on Tom's comment, I have made the scope a little broader. If a
function's "Datum *" parameter is changed to const, then if it has a
pairing "bool *isnull" parameter, I make it const as well. Also, if the
function has other pointer parameters that can be const, I change them to
const as well. See v3 attached.

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

  8. Re: Mark function arguments of type "Datum *" as "const Datum *" where possible

    Chao Li <li.evan.chao@gmail.com> — 2025-09-28T03:21:10Z

    Hi Nathan,

Thanks for your comment.

> On Sep 26, 2025, at 22:30, Nathan Bossart <nathandbossart@gmail.com> wrote:
> 
> I have mixed feelings about this patch.  I have no concrete objections to
> the technical content, but some questions come to mind.  For example, why
> are we only fixing Datum parameters?  Are we going to fix other types
> later?  Also, I'm a bit worried that this will lead to several rounds of
> similar patches, which IMHO is arguably unnecessary churn that could lead
> to back-patching pain.  I personally would be more likely to fix these
> sorts of things in passing as part of another patch that touches the same
> area of code.
> 


My initial motivation of creating this patch was from when I read the SPI code, for example:

/* Execute a previously prepared plan */
int
SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
                 bool read_only, long tcount)
{
    SPIExecuteOptions options;
    int         res;

This function define Nulls as const, but Values, which led me an impression that Values is an output parameter. But after reading through the function, I realized that Values should also be const. I believe other code readers might get the same confusing. 

I always raise comments while reviewing patches when a pointer parameter can be const but not. In the meantime, I wanted to start an effort to cleanup the confusion. Making pointer parameter “const” when possible not only makes the code safer, but also enhances code readability. As definitely we cannot update all code in a single commit, and as “Datum *” is the most typical one, I started with “Datum *”.

As the change only touches function definitions, I guess this patch itself doesn’t need to be back ported. When other patches are back ported to branches, and hitting the change from this patch, adding a few “const” to function parameters won’t be a big overhead.

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

  9. Re: Mark function arguments of type "Datum *" as "const Datum *" where possible

    Peter Eisentraut <peter@eisentraut.org> — 2025-10-31T09:56:35Z

    On 28.09.25 05:02, Chao Li wrote:
> 
> On Fri, Sep 26, 2025 at 11:00 PM Tom Lane <tgl@sss.pgh.pa.us 
> <mailto:tgl@sss.pgh.pa.us>> wrote:
> 
>     Yeah.  In particular, probably 99% of such Datum arrays also have an
>     associated array of bool isnull flags.  IMO it makes exactly zero
>     sense to const-ify the Datums without similar protection for their
>     isnull flags.
> 
> 
> Based on Tom's comment, I have made the scope a little broader. If a 
> function's "Datum *" parameter is changed to const, then if it has a 
> pairing "bool *isnull" parameter, I make it const as well. Also, if the 
> function has other pointer parameters that can be const, I change them 
> to const as well. See v3 attached.

I have committed this.  Note that the documentation for the SPI 
functions needed to be updated.

As discussed in the other thread about ItemPointer etc., I removed from 
your patch changes that would have broken externally usable callback 
function signatures.  But that was only a small portion of your patch.

  10. Re: Mark function arguments of type "Datum *" as "const Datum *" where possible

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-09T16:27:04Z

    Hi,

On Fri, Oct 31, 2025 at 10:56:35AM +0100, Peter Eisentraut wrote:
> On 28.09.25 05:02, Chao Li wrote:
> > 
> > On Fri, Sep 26, 2025 at 11:00 PM Tom Lane <tgl@sss.pgh.pa.us
> > <mailto:tgl@sss.pgh.pa.us>> wrote:
> > 
> >     Yeah.  In particular, probably 99% of such Datum arrays also have an
> >     associated array of bool isnull flags.  IMO it makes exactly zero
> >     sense to const-ify the Datums without similar protection for their
> >     isnull flags.
> > 
> > 
> > Based on Tom's comment, I have made the scope a little broader. If a
> > function's "Datum *" parameter is changed to const, then if it has a
> > pairing "bool *isnull" parameter, I make it const as well. Also, if the
> > function has other pointer parameters that can be const, I change them
> > to const as well. See v3 attached.
> 
> I have committed this. 

FWIW, I just submitted a patch [1] that extends this approach to all pointer types.

[1]: https://www.postgresql.org/message-id/flat/aThMaTPjSeFq3qTf@ip-10-97-1-34.eu-west-3.compute.internal

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com