Re: BETWEEN Node & DROP COLUMN

> Christopher, if you are still having trouble adding the isdropped system
> column, please let me know.

Thanks Bruce, but I think I've got it sorted now. One weird thing is that
although I added it as being false in pg_attribute.h, I get these tuples
having attisdropped set to true by initdb.

Are these from the toasting process and maybe the stats or something??

Chris

attrelid | attname
----------+-------------------
16464 | chunk_id
16464 | chunk_seq
16464 | chunk_data
16466 | chunk_id
16466 | chunk_seq
16467 | chunk_id
16467 | chunk_seq
16467 | chunk_data
16469 | chunk_id
16469 | chunk_seq
16470 | chunk_id
16470 | chunk_seq
16470 | chunk_data
16472 | chunk_id
16472 | chunk_seq
16473 | chunk_id
16473 | chunk_seq
16473 | chunk_data
16475 | chunk_id
16475 | chunk_seq
16476 | chunk_id
16476 | chunk_seq
16476 | chunk_data
16478 | chunk_id
16478 | chunk_seq
16479 | chunk_id
16479 | chunk_seq
16479 | chunk_data
16481 | chunk_id
16481 | chunk_seq
16482 | chunk_id
16482 | chunk_seq
16482 | chunk_data
16484 | chunk_id
16484 | chunk_seq
16485 | chunk_id
16485 | chunk_seq
16485 | chunk_data
16487 | chunk_id
16487 | chunk_seq
16488 | chunk_id
16488 | chunk_seq
16488 | chunk_data
16490 | chunk_id
16490 | chunk_seq
16491 | usecreatedb
16491 | usesuper
16491 | passwd
16491 | valuntil
16491 | useconfig
16494 | schemaname
16494 | tablename
16494 | rulename
16494 | definition
16498 | schemaname
16498 | viewname
16498 | viewowner
16498 | definition
16501 | tablename
16501 | tableowner
16501 | hasindexes
16501 | hasrules
16501 | hastriggers
16504 | tablename
16504 | indexname
16504 | indexdef
16507 | tablename
16507 | attname
16507 | null_frac
16507 | avg_width
16507 | n_distinct
16507 | most_common_vals
16507 | most_common_freqs
16507 | histogram_bounds
16507 | correlation
16511 | relid
16511 | relname
16511 | seq_scan
16511 | seq_tup_read
16511 | idx_scan
16511 | idx_tup_fetch
16511 | n_tup_ins
16511 | n_tup_upd
16511 | n_tup_del
16514 | relid
16514 | relname
16514 | heap_blks_read
16514 | heap_blks_hit
16514 | idx_blks_read
16514 | idx_blks_hit
16514 | toast_blks_read
16514 | toast_blks_hit
16514 | tidx_blks_read
16514 | tidx_blks_hit
16518 | relid
16518 | indexrelid
16518 | relname
16518 | indexrelname
16518 | idx_scan
16518 | idx_tup_read
16518 | idx_tup_fetch
16521 | relid
16521 | indexrelid
16521 | relname
16521 | indexrelname
16521 | idx_blks_read
16521 | idx_blks_hit
16524 | relid
16524 | relname
16524 | blks_read
16524 | blks_hit
16527 | datid
16527 | datname
16527 | procpid
16527 | usesysid
16527 | usename
16527 | current_query
16530 | datid
16530 | datname
16530 | numbackends
16530 | xact_commit
16530 | xact_rollback
16530 | blks_read
16530 | blks_hit

The problem is that the new column is now part of pg_attribute so every
catalog/pg_attribute.h DATA() line has to be updated. Did you update
them all with 'false' in the right slot? Not sure what the chunks are.

---------------------------------------------------------------------------

Christopher Kings-Lynne wrote:
> > Christopher, if you are still having trouble adding the isdropped system
> > column, please let me know.
>
> Thanks Bruce, but I think I've got it sorted now. One weird thing is that
> although I added it as being false in pg_attribute.h, I get these tuples
> having attisdropped set to true by initdb.
>
> Are these from the toasting process and maybe the stats or something??
>
> Chris
>
> attrelid | attname
> ----------+-------------------
> 16464 | chunk_id
> 16464 | chunk_seq
> 16464 | chunk_data
> 16466 | chunk_id
> 16466 | chunk_seq
> 16467 | chunk_id
> 16467 | chunk_seq
> 16467 | chunk_data
> 16469 | chunk_id
> 16469 | chunk_seq
> 16470 | chunk_id
> 16470 | chunk_seq
> 16470 | chunk_data
> 16472 | chunk_id
> 16472 | chunk_seq
> 16473 | chunk_id
> 16473 | chunk_seq
> 16473 | chunk_data
> 16475 | chunk_id
> 16475 | chunk_seq
> 16476 | chunk_id
> 16476 | chunk_seq
> 16476 | chunk_data
> 16478 | chunk_id
> 16478 | chunk_seq
> 16479 | chunk_id
> 16479 | chunk_seq
> 16479 | chunk_data
> 16481 | chunk_id
> 16481 | chunk_seq
> 16482 | chunk_id
> 16482 | chunk_seq
> 16482 | chunk_data
> 16484 | chunk_id
> 16484 | chunk_seq
> 16485 | chunk_id
> 16485 | chunk_seq
> 16485 | chunk_data
> 16487 | chunk_id
> 16487 | chunk_seq
> 16488 | chunk_id
> 16488 | chunk_seq
> 16488 | chunk_data
> 16490 | chunk_id
> 16490 | chunk_seq
> 16491 | usecreatedb
> 16491 | usesuper
> 16491 | passwd
> 16491 | valuntil
> 16491 | useconfig
> 16494 | schemaname
> 16494 | tablename
> 16494 | rulename
> 16494 | definition
> 16498 | schemaname
> 16498 | viewname
> 16498 | viewowner
> 16498 | definition
> 16501 | tablename
> 16501 | tableowner
> 16501 | hasindexes
> 16501 | hasrules
> 16501 | hastriggers
> 16504 | tablename
> 16504 | indexname
> 16504 | indexdef
> 16507 | tablename
> 16507 | attname
> 16507 | null_frac
> 16507 | avg_width
> 16507 | n_distinct
> 16507 | most_common_vals
> 16507 | most_common_freqs
> 16507 | histogram_bounds
> 16507 | correlation
> 16511 | relid
> 16511 | relname
> 16511 | seq_scan
> 16511 | seq_tup_read
> 16511 | idx_scan
> 16511 | idx_tup_fetch
> 16511 | n_tup_ins
> 16511 | n_tup_upd
> 16511 | n_tup_del
> 16514 | relid
> 16514 | relname
> 16514 | heap_blks_read
> 16514 | heap_blks_hit
> 16514 | idx_blks_read
> 16514 | idx_blks_hit
> 16514 | toast_blks_read
> 16514 | toast_blks_hit
> 16514 | tidx_blks_read
> 16514 | tidx_blks_hit
> 16518 | relid
> 16518 | indexrelid
> 16518 | relname
> 16518 | indexrelname
> 16518 | idx_scan
> 16518 | idx_tup_read
> 16518 | idx_tup_fetch
> 16521 | relid
> 16521 | indexrelid
> 16521 | relname
> 16521 | indexrelname
> 16521 | idx_blks_read
> 16521 | idx_blks_hit
> 16524 | relid
> 16524 | relname
> 16524 | blks_read
> 16524 | blks_hit
> 16527 | datid
> 16527 | datname
> 16527 | procpid
> 16527 | usesysid
> 16527 | usename
> 16527 | current_query
> 16530 | datid
> 16530 | datname
> 16530 | numbackends
> 16530 | xact_commit
> 16530 | xact_rollback
> 16530 | blks_read
> 16530 | blks_hit
>
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
>
>
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

"Christopher Kings-Lynne" <chriskl(at)familyhealth(dot)com(dot)au> writes:
> Thanks Bruce, but I think I've got it sorted now. One weird thing is that
> although I added it as being false in pg_attribute.h, I get these tuples
> having attisdropped set to true by initdb.

It sounds to me like you've failed to make sure that the field is
initialized properly when a pg_attribute row is dynamically created.
Let's see... did you fix the static FormData_pg_attribute rows near
the top of heap.c? Does TupleDescInitEntry() know about initializing
the field? (I wonder why it doesn't memset() the whole row to zero
anyway...)

pg_attribute is very possibly the most ticklish system catalog
to add a column to. I'd suggest looking through every single use of
some other pg_attribute column, perhaps attstattarget or attnotnull,
to make sure you're initializing attisdropped everywhere it should be.

regards, tom lane

> The problem is that the new column is now part of pg_attribute so every
> catalog/pg_attribute.h DATA() line has to be updated. Did you update
> them all with 'false' in the right slot? Not sure what the chunks are.

Yep - I did that, I think the problem's more subtle.

Chris

> It sounds to me like you've failed to make sure that the field is
> initialized properly when a pg_attribute row is dynamically created.
> Let's see... did you fix the static FormData_pg_attribute rows near
> the top of heap.c? Does TupleDescInitEntry() know about initializing
> the field? (I wonder why it doesn't memset() the whole row to zero
> anyway...)

OK I'll look at them.

> pg_attribute is very possibly the most ticklish system catalog
> to add a column to. I'd suggest looking through every single use of
> some other pg_attribute column, perhaps attstattarget or attnotnull,
> to make sure you're initializing attisdropped everywhere it should be.

OK, I'm on the case.

Chris

> > pg_attribute is very possibly the most ticklish system catalog
> > to add a column to. I'd suggest looking through every single use of
> > some other pg_attribute column, perhaps attstattarget or attnotnull,
> > to make sure you're initializing attisdropped everywhere it should be.

Done.

Wow - I've almost finished it now, actually! It's at the stage where
everything works as expected, all the initdb attributes are properly marked
not dropped, the drop column command works fine and psql works fine. All
regression tests also pass. '*' expansion works properly.

I have a lot of testing to go, however. I will make up regression tests as
well.

Some questions:

1. I'm going to prevent people from dropping the last column in their table.
I think this is the safest option. How do I check if there's any other non
dropped columns in a table? Reference code anywhere?

2. What should I do about inheritance? I'm going to implement it, but are
there issues? It will basically drop the column with the same name in all
child tables. Is that correct behaviour?

3. I am going to initially implement the patch to ignore the behaviour and
do no dependency checking. I will assume that Rod's patch will handle that
without much trouble.

Thanks,

Chris

Christopher Kings-Lynne dijo:

I have a question:

> 2. What should I do about inheritance? I'm going to implement it, but are
> there issues? It will basically drop the column with the same name in all
> child tables. Is that correct behaviour?

What happens if I drop an inherited column in a child table? Maybe it
works, but what happens when I SELECT the column in the parent table?

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"Granting software the freedom to evolve guarantees only different results,
not better ones." (Zygo Blaxell)

> > 2. What should I do about inheritance? I'm going to implement
> it, but are
> > there issues? It will basically drop the column with the same
> name in all
> > child tables. Is that correct behaviour?
>
> What happens if I drop an inherited column in a child table? Maybe it
> works, but what happens when I SELECT the column in the parent table?

I don't know? Tom?

Well, what happens if you rename a column in a child table? Same problem?

Chris

Christopher Kings-Lynne dijo:

> > > 2. What should I do about inheritance? I'm going to implement
> > it, but are
> > > there issues? It will basically drop the column with the same
> > name in all
> > > child tables. Is that correct behaviour?
> >
> > What happens if I drop an inherited column in a child table? Maybe it
> > works, but what happens when I SELECT the column in the parent table?
>
> I don't know? Tom?
>
> Well, what happens if you rename a column in a child table? Same problem?

It merrily renames the column in the child table (I tried it). When
SELECTing the parent, bogus data appears. Looks like a bug to me.
Maybe the ALTER TABLE ... RENAME COLUMN code should check for inherited
columns before renaming them.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
One man's impedance mismatch is another man's layer of abstraction.
(Lincoln Yeoh)

> > Well, what happens if you rename a column in a child table?
> Same problem?
>
> It merrily renames the column in the child table (I tried it). When
> SELECTing the parent, bogus data appears. Looks like a bug to me.
> Maybe the ALTER TABLE ... RENAME COLUMN code should check for inherited
> columns before renaming them.

Hmmm...so how does one check if one is a child in an inheritance hierarchy?

Chris

> > It merrily renames the column in the child table (I tried it). When
> > SELECTing the parent, bogus data appears. Looks like a bug to me.
> > Maybe the ALTER TABLE ... RENAME COLUMN code should check for inherited
> > columns before renaming them.
>
> Hmmm...so how does one check if one is a child in an inheritance
> hierarchy?

Actually, more specifically, how does one check that the column being
dropped or renamed appears in none of one's parent tables?

I notice there's no find_all_ancestors() function...

Chris

"Christopher Kings-Lynne" <chriskl(at)familyhealth(dot)com(dot)au> writes:
>> What happens if I drop an inherited column in a child table? Maybe it
>> works, but what happens when I SELECT the column in the parent table?

> Well, what happens if you rename a column in a child table? Same problem?

Ideally we should disallow both of those, as well as cases like
changing the column type.

It might be that we can use the pg_depend stuff to enforce this (by
setting up dependency links from child to parent). However that would
introduce a ton of overhead in a regular DROP TABLE, and you'd still
need specialized code to prevent the RENAME case (pg_depend wouldn't
care about that). I'm thinking that it's worth adding an attisinherited
column to pg_attribute to make these rules easy to enforce.

regards, tom lane

"Christopher Kings-Lynne" <chriskl(at)familyhealth(dot)com(dot)au> writes:
> 1. I'm going to prevent people from dropping the last column in their table.
> I think this is the safest option. How do I check if there's any other non
> dropped columns in a table? Reference code anywhere?

You look through the Relation's tupledesc and make sure there's at least
one other non-dropped column.

> 2. What should I do about inheritance? I'm going to implement it, but are
> there issues? It will basically drop the column with the same name in all
> child tables. Is that correct behaviour?

Yes, if the 'inh' flag is set.

If 'inh' is not set, then the right thing would be to drop the parent's
column and mark all the *first level* children's columns as
not-inherited. How painful that would be depends on what representation
we choose for marking inherited columns, if any.

> 3. I am going to initially implement the patch to ignore the behaviour and
> do no dependency checking. I will assume that Rod's patch will handle that
> without much trouble.

Yeah, Rod was looking ahead to DROP COLUMN. I'm still working on his
patch (mostly the pg_constraint side) but should have it soon.

regards, tom lane