Re: [HACKERS] Partitioned tables and relfilenode - Mailing list pgsql-hackers

Thanks for the review.  I was just about to send a new version of the patches.

On 2017/02/28 12:20, Robert Haas wrote:
> On Thu, Feb 23, 2017 at 11:19 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Thanks for the review.
> 
> In 0001, the documentation which you are patching has a section for
> limitations that apply only to both partitioning and constraint
> exclusion, and another for limitations that apply only to constraint
> exclusion.  Surely the patch should be moving a limitation that will
> no longer apply to partitioning from the first section to the second
> section, rather than leaving it in the section for limitations that
> apply to both systems and just adding a note that say "this doesn't
> apply to partitioning any more".

So we have the following headings under 5.11.6 Caveats:

"The following caveats apply to partitioned tables implemented using
either method (unless noted otherwise)"

and,

"The following caveats apply to constraint exclusion"

The latter lists only the caveats about the planner capability (constraint
exclusion) for partitioning - that it cannot be performed against volatile
WHERE conditions, only works with comparisons using btree operators, does
not scale beyond hundred partitions, etc.  These limitations are common to
both systems (inheritance and declarative partitioned tables), because
both rely on constraint exclusion.

How about separating the limitations listed under the first heading into:

"Limitations of using inheritance for partitioning"

and,

"Limitations of using partitioned tables"

This particular patch gets rid of the restriction that vacuum/analyze do
not recurse to child tables (partitions) only for the second of the above.

How about the documentation changes in the attached updated 0001?  I know
that this page needs a much larger rewrite as we are discussing in the
other thread.

> In acquire_inherited_sample_rows(), instead of inserting a whole
> stanza of logic just above the existing dispatch on relkind, I think
> we can get by with a very slightly update to what's already there.
> 
> You can't use the result of a & b as a bool.  You need to write (a &
> b) != 0, because the bool should always use 1 for true and 0 for
> false; it should not set some higher-numbered bit.

Oops, thanks for fixing that.  I suppose you are referring to this hunk in
the original patch:

-    relations = get_rel_oids(relid, relation);
+    relations = get_rel_oids(relid, relation, options & VACOPT_VACUUM);

And we need to do it this way in *this* case, because we're passing it as
a bool argument.  I see that it's OK to do this:

    stmttype = (options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";

Or this:

    if (options & VACOPT_VACUUM)
    {
        PreventTransactionChain(isTopLevel, stmttype);

> The changes to autovacuum.c look useless, because
> relation_needs_vacanalyze() will presumably never fire for a table
> with no tuples of its own.

Hmm, I guess that makes sense.  Inheritance statistics are never collected
by autovacuum (at least in the partitioning setups and especially with the
partitioned tables now).  I recall that you mentioned [1] this as one of
the limitations of the system currently.

> Updated patch with those changes and a few cosmetic tweaks attached.

I have incorporated the changes in your patch in the attached updated
0001, with documentation tweaks as mentioned above.

Other than that, in acquire_inherited_sample_rows() I've used a method
suggested by Ashutosh Bapat to track if we encountered a child table,
which is much less ugly than below:

-    if (nrels < 2)
+    if ((onerel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE && nrels <
2) ||
+        nrels < 1)


I keep updated 0002 and 0003 here, but I've changed their order.  Don't
scan partitioned tables thing is now 0002 and don't allocate file for
partitioned tables is 0003.  Sorry about the confusion.

Thanks,
Amit

[1]
https://postgr.es/m/CA%2BTgmobTxn2%2B0x96h5Le%2BGOK5kw3J37SRveNfzEdx9s5-Yd8vA%40mail.gmail.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers