Hi Jeevan, On 2017/05/30 16:38, Jeevan Ladhe wrote: > I have rebased the patch on the latest commit. > PFA.
Was looking at the patch and felt that the parse node representation of default partition bound could be slightly different. Can you explain the motivation behind implementing it without adding a new member to the PartitionBoundSpec struct? I would suggest instead adding a bool named is_default and be done with it. It will help get rid of the public isDefaultPartitionBound() in the proposed patch whose interface isn't quite clear and instead simply check if (spec->is_default) in places where it's called by passing it (Node *) linitial(spec->listdatums). Further looking into the patch, I found a tiny problem in check_default_allows_bound(). If the default partition that will be scanned by it is a foreign table or a partitioned table with a foreign leaf partition, you will get a failure like: -- default partition is a foreign table alter table p attach partition fp default; -- adding a new partition will try to scan fp above alter table p attach partition p12 for values in (1, 2); ERROR: could not open file "base/13158/16456": No such file or directory I think the foreign tables should be ignored here to avoid the error. The fact that foreign default partition may contain data that satisfies the new partition's constraint is something we cannot do much about. Also, see the note in ATTACH PARTITION description regarding foreign tables [1] and the discussion at [2]. Thanks, Amit [1] https://www.postgresql.org/docs/devel/static/sql-altertable.html [2] https://www.postgresql.org/message-id/flat/8f89dcb2-bd15-d8dc-5f54-3e11dc6c9463%40lab.ntt.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
