Thread: attndims, typndims still not enforced, but make the value within a sane threshold

hi.

while looking at tablecmd.c, BuildDescForRelation
        attdim = list_length(entry->typeName->arrayBounds);
        if (attdim > PG_INT16_MAX)
            ereport(ERROR,
                    errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
                    errmsg("too many array dimensions"))

makes me related to array_in refactor previously we did.
at first, i thought it should be "if (attdim > MAXDIM)"


pg_attribute attndims description in [1]
attndims int2
Number of dimensions, if the column is an array type; otherwise 0.
(Presently, the number of dimensions of an array is not enforced, so
any nonzero value effectively means “it's an array”.)

pg_type typndims description in [2]
typndims int4
typndims is the number of array dimensions for a domain over an array
(that is, typbasetype is an array type). Zero for types other than
domains over array types.

since array_in is the only source of the real array data.
MAXDIM (6) ensure the max dimension is 6.

Can we error out at the stage "create table", "create domain"
time if the attndims or typndims is larger than MAXDIM (6) ?

for example, error out the following queries  immediately
create table t112(a int[][] [][] [][] [][][]);
create domain d_text_arr text [1][][][][][][][];

in the doc, we can still say "the number of dimensions of an array is
not enforced",
but attndims, typndims value would be within a sane threshold.

We can change typndims from int4 to int2,
so array type's dimension is consistent with domain type's dimension.
but it seems with the change, pg_type occupies the same amount of
storage as int4.


[1] https://www.postgresql.org/docs/current/catalog-pg-attribute.html
[2] https://www.postgresql.org/docs/current/catalog-pg-type.html

jian he <jian.universality@gmail.com> writes:
> Can we error out at the stage "create table", "create domain"
> time if the attndims or typndims is larger than MAXDIM (6) ?

The last time this was discussed, I think the conclusion was
we should remove attndims and typndims entirely on the grounds
that they're useless.  I certainly don't see a point in adding
more logic that could give the misleading impression that they
mean something.

            regards, tom lane

On Wed, Sep 18, 2024 at 10:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> jian he <jian.universality@gmail.com> writes:
> > Can we error out at the stage "create table", "create domain"
> > time if the attndims or typndims is larger than MAXDIM (6) ?
>
> The last time this was discussed, I think the conclusion was
> we should remove attndims and typndims entirely on the grounds
> that they're useless.  I certainly don't see a point in adding
> more logic that could give the misleading impression that they
> mean something.
>

https://commitfest.postgresql.org/43/
search "dim" or "pg_attribute", no relevant result,
i am assuming, nobody doing work to remove attndims and typndims entirely?
If so, I will try to make one.

On Fri, Sep 20, 2024 at 10:11 AM jian he <jian.universality@gmail.com> wrote:
>
> On Wed, Sep 18, 2024 at 10:35 PM jian he <jian.universality@gmail.com> wrote:
> >
> > > The last time this was discussed, I think the conclusion was
> > > we should remove attndims and typndims entirely on the grounds
> > > that they're useless.  I certainly don't see a point in adding
> > > more logic that could give the misleading impression that they
> > > mean something.
> > >
>
>
> attached patch removes attndims and typndims entirely.
> some tests skipped in my local my machine, not skipped are all OK.

Should you also bump the catalog version?

--
Regards
Junwang Zhao

Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Sep 20, 2024 at 11:51:49AM +0800, Junwang Zhao wrote:
>> Should you also bump the catalog version?

> No need to worry about that when sending a patch because committers
> take care of that when merging a patch into the tree.  Doing that in
> each patch submitted just creates more conflicts and work for patch
> authors because they'd need to recolve conflicts each time a
> catversion bump happens.  And that can happen on a daily basis
> sometimes depending on what is committed.

Right.  Sometimes the committer forgets to do that :-(, which is
not great but it's not normally a big problem either.  We've concluded
it's better to err in that direction than impose additional work
on patch submitters.

If you feel concerned about the point, best practice is to include a
mention that catversion bump is needed in your draft commit message.

            regards, tom lane

Hi Tom and Michael,

On Fri, Sep 20, 2024 at 12:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Michael Paquier <michael@paquier.xyz> writes:
> > On Fri, Sep 20, 2024 at 11:51:49AM +0800, Junwang Zhao wrote:
> >> Should you also bump the catalog version?
>
> > No need to worry about that when sending a patch because committers
> > take care of that when merging a patch into the tree.  Doing that in
> > each patch submitted just creates more conflicts and work for patch
> > authors because they'd need to recolve conflicts each time a
> > catversion bump happens.  And that can happen on a daily basis
> > sometimes depending on what is committed.
>
> Right.  Sometimes the committer forgets to do that :-(, which is
> not great but it's not normally a big problem either.  We've concluded
> it's better to err in that direction than impose additional work
> on patch submitters.
>
> If you feel concerned about the point, best practice is to include a
> mention that catversion bump is needed in your draft commit message.
>
>                         regards, tom lane

Got it, thanks for both of your explanations.

--
Regards
Junwang Zhao

On Fri, Sep 20, 2024 at 10:11:00AM +0800, jian he wrote:
> On Wed, Sep 18, 2024 at 10:35 PM jian he <jian.universality@gmail.com> wrote:
> >
> > > The last time this was discussed, I think the conclusion was
> > > we should remove attndims and typndims entirely on the grounds
> > > that they're useless.  I certainly don't see a point in adding
> > > more logic that could give the misleading impression that they
> > > mean something.
> > >
> 
> 
> attached patch removes attndims and typndims entirely.
> some tests skipped in my local my machine, not skipped are all OK.

I have been hoping for a patch links this because I feel the existence
of these system columns is deceptive since we don't honor them properly.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"

On 2024-09-20 Fr 12:38 AM, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> On Fri, Sep 20, 2024 at 11:51:49AM +0800, Junwang Zhao wrote:
>>> Should you also bump the catalog version?
>> No need to worry about that when sending a patch because committers
>> take care of that when merging a patch into the tree.  Doing that in
>> each patch submitted just creates more conflicts and work for patch
>> authors because they'd need to recolve conflicts each time a
>> catversion bump happens.  And that can happen on a daily basis
>> sometimes depending on what is committed.
> Right.  Sometimes the committer forgets to do that :-(, which is
> not great but it's not normally a big problem either.  We've concluded
> it's better to err in that direction than impose additional work
> on patch submitters.


FWIW, I have a git pre-commit hook that helps avoid that. Essentially it 
checks to see if there are changes in src/include/catalog but not in 
catversion.h. That's not a 100% check, but it probably catches the vast 
majority of changes that would require a catversion bump.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com