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