Re: partitioning and identity column - Mailing list pgsql-hackers

> 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@gmail.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.

     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?

* 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?