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

From Robert Haas
Subject Re: [HACKERS] Partitioned tables and relfilenode
Date
Msg-id CA+TgmoaDj0MRpSFkQ3xSpSbC7-7oS-XNzQkFfcEs6=-YrJuHwQ@mail.gmail.com
Whole thread Raw
In response to Re: [HACKERS] Partitioned tables and relfilenode  (Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>)
Responses Re: [HACKERS] Partitioned tables and relfilenode  (Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>)
List pgsql-hackers
On Tue, Feb 28, 2017 at 11:35 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> 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.

Looks good.

>> 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);

In those cases it's still clearer, IMHO, to use != 0, but it's not
necessary.  However, when you're explicitly creating a value of type
"bool", then it's necessary.

Actually, looking at this again, I now think this part is wrong:

+            /*
+             * If only ANALYZE is to be performed, there is no need to include
+             * partitions in the list.  In a database-wide ANALYZE, we only
+             * update the inheritance statistics of partitioned tables, not
+             * the statistics of individual partitions.
+             */
+            if (!is_vacuum && classForm->relispartition)                continue;

I was thinking earlier that an ANALYZE on the parent would also update
the statistics for each child, but now I see that's not so.  So now I
think we should omit this logic (and change the documentation to
match).  That is, a database-wide ANALYZE should update the statistics
for each child as well as for the parent.  Otherwise direct queries
against the children (and partitionwise joins, once we have that) are
going to go haywire.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



pgsql-hackers by date:

Previous
From: Surafel Temesgen
Date:
Subject: Re: [HACKERS] Disallowing multiple queries per PQexec()
Next
From: Robert Haas
Date:
Subject: Re: [HACKERS] Re: new high availability feature for the system withboth asynchronous and synchronous replication