CONSTANT/NOT NULL/initializer properties for plpgsql record variables

I wrote:

I said a couple of times in recent threads that it wouldn't be too hard
to implement $SUBJECT given the other patches I've been working on.

Here's a version rebased up to HEAD, with a trivial merge conflict fixed.

This now needs to be applied over the patches in
/messages/by-id/833.1516834367@sss.pgh.pa.us
and
/messages/by-id/8376.1516835784@sss.pgh.pa.us

regards, tom lane

On 25 Jan 2018, at 00:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

I said a couple of times in recent threads that it wouldn't be too hard
to implement $SUBJECT given the other patches I've been working on.

Here's a version rebased up to HEAD, with a trivial merge conflict fixed.

This now needs to be applied over the patches in
/messages/by-id/833.1516834367@sss.pgh.pa.us
and
/messages/by-id/8376.1516835784@sss.pgh.pa.us

I’ve reviewed this patch (disclaimer: I did not review the patches listed above
which it is based on) and the functionality introduced. The code is straight-
forward, there are ample tests and I can’t make it break however many weird
combinations thrown at it. Regarding the functionality it’s clearly a +1 on
getting this in.

One tiny thing: while not introduced in this patch, I wonder if it would be
worth adding an errhint in the following hunk when applied to arrays, to
clarify what CONSTANT in an array declaration mean. I have seen confusion
around what y means in ‘x int[y]’, and I can see ‘x CONSTANT int[y]’ being
misinterpreted as “length fixed to y”.

-                        errmsg("\"%s\" is declared CONSTANT",
-                               ((PLpgSQL_var *) datum)->refname),
+                        errmsg("variable \"%s\" is declared CONSTANT",
+                               ((PLpgSQL_variable *) datum)->refname),

That might not be a common enough misunderstanding to warrant it, but it was
the only thing that stood out to me (and perhaps it would be better in the docs
if done at all).

Setting this ready for committer, with the caveat that the underlying patches
must go in of course.

cheers ./daniel

Daniel Gustafsson <daniel@yesql.se> writes:

I’ve reviewed this patch (disclaimer: I did not review the patches listed above
which it is based on) and the functionality introduced. The code is straight-
forward, there are ample tests and I can’t make it break however many weird
combinations thrown at it. Regarding the functionality it’s clearly a +1 on
getting this in.

Thanks for reviewing!

One tiny thing: while not introduced in this patch, I wonder if it would be
worth adding an errhint in the following hunk when applied to arrays, to
clarify what CONSTANT in an array declaration mean. I have seen confusion
around what y means in ‘x int[y]’, and I can see ‘x CONSTANT int[y]’ being
misinterpreted as “length fixed to y”.

Hmm. What would you imagine the hint saying? It's certainly true that
a lot of people don't understand that a declared array length doesn't
mean anything in Postgres, but that's not specific to this context.

That might not be a common enough misunderstanding to warrant it, but it was
the only thing that stood out to me (and perhaps it would be better in the docs
if done at all).

The documentation currently says

The CONSTANT option prevents the variable from being assigned to
after initialization, so that its value will remain constant for
the duration of the block.

I don't mind changing that if we can think of clearer wording, but to
me it seems pretty clear already.

regards, tom lane

On Thursday, January 25, 2018, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The documentation currently says

The CONSTANT option prevents the variable from being assigned to
after initialization, so that its value will remain constant for
the duration of the block.

While we don't really have the concept of mutable types other languages
do. Maybe just saying "from being assigned or mutated after
initialization" would suffice.

I don't see a desirable way to reinforce that the y in arr[y] is unenforced
documentation - in the code or docs - beyond what is already done. That
distinction doesn't usually come up and for those where it does the docs
will probably be an after-the-fact confirmation of observed behavior.

David J.

On 25 Jan 2018, at 16:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:

One tiny thing: while not introduced in this patch, I wonder if it would be
worth adding an errhint in the following hunk when applied to arrays, to
clarify what CONSTANT in an array declaration mean. I have seen confusion
around what y means in ‘x int[y]’, and I can see ‘x CONSTANT int[y]’ being
misinterpreted as “length fixed to y”.

Hmm. What would you imagine the hint saying? It's certainly true that
a lot of people don't understand that a declared array length doesn't
mean anything in Postgres, but that's not specific to this context.

I was thinking about something along the lines of:

errhint("In array declarations, CONSTANT refers to elements and not length."),

.. when datatype->typisarray is true, but again it might just be waffling that
doesn’t help anyone.

cheers ./daniel