Re: Partitioned tables and relfilenode

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Cc: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Partitioned tables and relfilenode
Date: 2017-03-02 09:36:52
Message-ID: CA+TgmoaDj0MRpSFkQ3xSpSbC7-7oS-XNzQkFfcEs6=-YrJuHwQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Feb 28, 2017 at 11:35 AM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2017-03-02 09:49:06 Re: Re: new high availability feature for the system with both asynchronous and synchronous replication
Previous Message Surafel Temesgen 2017-03-02 09:01:17 Re: Disallowing multiple queries per PQexec()