I see that all the changes by Amit and myself to what was earlier 0003 patch are now part of 0002 patch. The patch looks ready for committer.
Some comments about 0003 patch. CREATE TABLE syntax seems to allow specifying reloptions for a partitioned table. But extractRelOptions() and heap_reloptions() seem to be ignoring those. Is that intentional? There are two callers of extractRelOptions(), extract_autovac_opts() and RelationParseRelOptions(). There's an assert in extract_autovac_opts() preventing that function from getting called for partitioned table. RelationParseRelOptions() seems to filter the relations before calling extractRelOptions(). I am wondering whether it's better to leave extractRelOptions() and filter partitioned relations in RelationParseRelOptions(). Do we need similar condition modification in the other caller of i.e. heap_create_init_fork(), ExecuteTruncate()? On Mon, Mar 6, 2017 at 12:48 PM, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > Thanks for the review. > > On 2017/03/06 15:41, Michael Paquier wrote: >> On Fri, Mar 3, 2017 at 10:02 AM, Amit Langote >> <langote_amit...@lab.ntt.co.jp> wrote: >>> Thanks. I noticed that 'and' is duplicated in a line added by the commit >>> to analyze.sgml. Attached 0001 fixes that. 0002 and 0003 same as the >>> last version. >> >> /* >> - * If all the children were temp tables, pretend it's a non-inheritance >> - * situation. The duplicate RTE we added for the parent table is >> - * harmless, so we don't bother to get rid of it; ditto for the useless >> - * PlanRowMark node. >> + * If all the children were temp tables or if the parent is a partitioned >> + * table without any leaf partitions, pretend it's a non-inheritance >> + * situation. The duplicate RTE for the parent table we added in the >> + * non-partitioned table case is harmless, so we don't bother to get rid >> + * of it; ditto for the useless PlanRowMark node. >> */ >> - if (list_length(appinfos) < 2) >> + if (!has_child) >> This comment is not completely correct. Children can be temp tables, >> they just cannot be temp tables of other backends. It seems to me that >> you could still keep this code simple and remove has_child.. > > I updated the comment. I recall having posted a patch for that once, but > perhaps went unnoticed. > > About has_child, the other option is to make the minimum length of > appinfos list relkind-based, but the condition quickly becomes ugly. Do > you have a suggestion? > >> @@ -932,7 +932,6 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc, >> case RELKIND_RELATION: >> case RELKIND_TOASTVALUE: >> case RELKIND_MATVIEW: >> - case RELKIND_PARTITIONED_TABLE: >> options = heap_reloptions(classForm->relkind, datum, false); >> break; >> Partitioned tables cannot have reloptions? What about all the >> autovacuum_* parameters then? Those are mainly not storage-related. > > AFAIK, none of the heap reloptions will be applicable to partitioned table > relations once we eliminate storage. > > About autovacuum_* parameters - we currently don't handle partitioned > tables in autovacuum.c, because no statistics are reported for them. That > is, relation_needs_vacanalyze() will never return true for dovacuum, > doanalyze and wraparound if it is passed a RELKIND_PARTITIONED_TABLE > relation. That's something to be fixed separately though. When we add > autovacuum support for partitioned tables, we may want to add a new set of > reloptions (new because partitioned tables still won't support all options > returned by heap_reloptions()). Am I missing something? > > Thanks, > Amit -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
