Re: partitioning and identity column

On Sun, May 5, 2024 at 1:43 AM Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:

> > On Tue, Apr 30, 2024 at 04:29:11PM +0530, Ashutosh Bapat wrote:
> > On Sun, Apr 28, 2024 at 12:29 PM Alexander Lakhin <exclusion(at)gmail(dot)com>
> > wrote:
> >
> > > 27.04.2024 18:00, Alexander Lakhin wrote:
> > > >
> > > > Please look also at another script, which produces the same error:
> > >
> > > I've discovered yet another problematic case:
> > > CREATE TABLE tbl1 (a int GENERATED ALWAYS AS IDENTITY, b text)
> > > PARTITION BY LIST (a);
> > > CREATE TABLE tbl2 (b text, a int NOT NULL);
> > > ALTER TABLE tbl1 ATTACH PARTITION tbl2 DEFAULT;
> > >
> > > INSERT INTO tbl2 DEFAULT VALUES;
> > > ERROR: no owned sequence found
> > >
> > > Though it works with tbl2(a int NOT NULL, b text)...
> > > Take a look at this too, please.
> > >
> >
> > Thanks Alexander for the report.
> >
> > PFA patch which fixes all the three problems.
> >
> > I had not fixed getIdentitySequence() to fetch identity sequence
> associated
> > with the partition because I thought it would be better to fail with an
> > error when it's not used correctly. But these bugs show 1. the error is
> > misleading and unwanted 2. there are more places where adding that logic
> > to getIdentitySequence() makes sense. Fixed the function in these
> patches.
> > Now callers like transformAlterTableStmt have to be careful not to call
> the
> > function on a partition.
> >
> > I have examined all the callers of getIdentitySequence() and they seem to
> > be fine. The code related to SetIdentity, DropIdentity is not called for
> > partitions, errors for which are thrown elsewhere earlier.
>
> Thanks for the fix.
>
> I had a quick look, it covers the issues mentioned above in the thread.
> Few nitpicks/questions:
>
> * I think it makes sense to verify if the ptup is valid. This approach
> would fail if the target column of the root partition is marked as
> attisdropped.
>

The column is searched by name which is derived from attno of child
partition. So it has to exist in the root partition. If it doesn't
something is seriously wrong. Do you have a reproducer? We may want to add
Assert(HeapTupleIsValid(ptup)) just in case. But it seems unnecessary to me.

>
> Oid
> -getIdentitySequence(Oid relid, AttrNumber attnum, bool missing_ok)
> +getIdentitySequence(Relation rel, AttrNumber attnum, bool missing_ok)
> {
>
> [...]
>
> + relid = llast_oid(ancestors);
> + ptup = SearchSysCacheAttName(relid, attname);
> + attnum = ((Form_pg_attribute) GETSTRUCT(ptup))->attnum;
>
> * getIdentitySequence is used in build_column_default, which in turn
> often appears in loops over table attributes. AFAICT it means that the
> same root partition search will be repeated multiple times in such
> situations if there is more than one identity. I assume the
> performance impact of this repetition is negligible?
>

I thought having multiple identity columns would be rare and hence avoided
making code complex. Otherwise we have to get root partition somewhere in
the caller hierarchy separately the logic much farther apart. Usually the
ancestor entries will be somewhere in the cache

>
> * Maybe a silly question, but since I'm not aware about all the details
> here, I'm curious -- the approach of mapping attributes of a partition
> to the root partition attributes, how robust is it? I guess there is
> no way that the root partition column will be not what is expected,
> e.g. due to some sort of concurrency?
>

Any such thing would require a lock on the partition relation in the
question which is locked before passing rel around? So it shouldn't happen.

--
Best Wishes,
Ashutosh Bapat