Copy column storage parameters on CREATE TABLE LIKE/INHERITS

Lists: pgsql-hackers
From: ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
To: HACKERS <pgsql-hackers(at)postgresql(dot)org>
Subject: Copy column storage parameters on CREATE TABLE LIKE/INHERITS
Date: 2008-08-12 08:46:49
Message-ID: 20080812170932.8E65.52131E4D@oss.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Here is an updated version of the "Copy storage parameters" patch.
http://archives.postgresql.org/pgsql-hackers/2008-07/msg01417.php

This patch copies only column storage parameters and does not copy
reloptions as a result of the discussion, in which reloptions should
not be copied because LIKE and INHERITS are not table definitions,
but column definitions.

Copying reloptions (or copying just table's definition) might be useful
in some cases, but it should be done by another independent patch.

Inheriting column storage parameters is useful if we duplicate an
existing table. CREATE TABLE AS is useful to avoid WALs in that time:

CREATE TABLE (LIKE target) WITH (<same as the target table>)
AS SELECT * FROM target;

However, we cannot execute ALTER COLUMN SET STORAGE after creating the
new table and before inserting, because there are no time to execute
commands between them. So we need some methods to set column storage
parameters at creating a table.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Attachment Content-Type Size
inherits-column-params.patch application/octet-stream 7.5 KB

From: Stephen Frost <sfrost(at)snowman(dot)net>
To: pgsql-hackers(at)postgreSQL(dot)org
Cc: ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
Subject: [Patch Review] Copy column storage parameters on CREATE TABLE LIKE/INHERITS
Date: 2008-09-09 02:35:33
Message-ID: 20080909023533.GT16005@tamriel.snowman.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Greetings,

I've reviewed the patch here:
http://archives.postgresql.org/message-id/20080812170932.8E65.52131E4D@oss.ntt.co.jp

and am happy to report that it looks to be in good order. It has
documentation and regression updates, is in context diff format,
patches against current CVS with only some minor fuzz, and appears to
work as advertised. I looked over the patch and could easily follow
what was going on, did some additional testing beyond the regression
tests, and reviewed the documentation changes.

My only comment on this patch is that the documentation updates might
include a link back to Section 53.2 for more information, and/or to
the SET STORAGE portion of the alter table/alter column command
documentation, since the only other 'storage' reference in create
table's documentation is about table storage parameters.

Thanks!

Stephen


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
Cc: HACKERS <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Copy column storage parameters on CREATE TABLE LIKE/INHERITS
Date: 2008-09-11 20:33:26
Message-ID: 13146.1221165206@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
> Here is an updated version of the "Copy storage parameters" patch.
> http://archives.postgresql.org/pgsql-hackers/2008-07/msg01417.php

I looked this over and feel that it still needs work. The
implementation seems appropriate for columns coming from LIKE clauses,
but there are two big issues for columns coming from inheritance parent
tables:

* The patch opens and acquires lock on the parent relations far too
early. That is supposed to happen in DefineRelation, which among other
things contains the appropriate permissions check. It would be possible
to use this patch to hold AccessShareLock for a long time on tables you
have no permissions for.

* There is no consideration for merging of similarly-named columns
coming from different parent tables. The general approach taken in
DefineRelation is to throw an error if there are incompatible properties
in columns to be merged, and I think the same should happen for storage
properties. What you actually would get, here, is that some random one
of the columns would "win".

In short, I think you need two implementations, one for LIKE and one
for INHERITS, and the appropriate place to tackle the latter case is in
DefineRelation (or even more specifically, MergeAttributes).

Also I concur with Stephen Frost's suggestion to add a couple of
cross-references to the documentation patches.

regards, tom lane