From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Zhihong Yu <zyu@yugabyte.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: PoC/WIP: Extended statistics on expressions
Date: Mon, 18 Jan 2021 00:14:30 +0100
Message-ID: <01a2e09f-843a-824b-42c4-4d31d173b4a7@enterprisedb.com> (raw)
In-Reply-To: <CALNJ-vRz-hsCFVNQLm7Amxse+J=dLwSuwbJNeYOk-9mwvh_AyA@mail.gmail.com>
References: <CAEZATCXAPrrOKwEsyZKQ4uzzJQWBCt6QAvOcgqRGdWwT1zb+rQ@mail.gmail.com>
<329203a4-7cad-a0e3-d5a8-ce70872c8156@enterprisedb.com>
<CAEZATCWGYooC_Tzt8sQo7760ZEsguj0JGoMLhkANrDwJ9CY20Q@mail.gmail.com>
<2567ae7e-e1dd-48b5-054d-ed6ff81c0335@enterprisedb.com>
<CAEZATCW_7nfeHmLtV6TzSfmFG+i=io_Qo0NL8BiBnmEqq3CQnQ@mail.gmail.com>
<CAEZATCUfMM43sv+RFDqyczQpYMP7SZLd4EexFEEUX4buWRNnrw@mail.gmail.com>
<afcd65d7-7701-a469-b064-9c62d63ef437@enterprisedb.com>
<20210108023537.GA19743@telsasoft.com>
<d7c93270-20a0-93cb-f15e-46d7b51b35c1@enterprisedb.com>
<5305f9cf-3927-06c4-cd5b-e46cbc58bc7f@enterprisedb.com>
<20210116232208.GB8560@telsasoft.com>
<2f2a3b3b-93e4-723f-144e-bc21f10d6b61@enterprisedb.com>
<CALNJ-vRz-hsCFVNQLm7Amxse+J=dLwSuwbJNeYOk-9mwvh_AyA@mail.gmail.com>
On 1/17/21 3:55 AM, Zhihong Yu wrote:
> Hi,
> + * Check that only the base rel is mentioned. (This should be dead code
> + * now that add_missing_from is history.)
> + */
> + if (list_length(pstate->p_rtable) != 1)
>
> If it is dead code, it can be removed, right ?
>
Maybe. The question is whether it really is dead code. It's copied from
transformIndexStmt so I kept it for now.
> For statext_mcv_clauselist_selectivity:
>
> + // bms_free(list_attnums[listidx]);
> > The commented line can be removed.
>
Actually, this should probably do list_free on the list_exprs.
> +bool
> +examine_clause_args2(List *args, Node **exprp, Const **cstp, bool
> *expronleftp)
>
> Better add some comment for examine_clause_args2 since there
> is examine_clause_args() already.
>
Yeah, this probably needs a better name. I have a feeling this may need
a refactoring to reuse more of the existing code for the expressions.
> + if (!ok || stats->compute_stats == NULL || stats->minrows <= 0)
>
> When would stats->minrows have negative value ?
>
The typanalyze function (e.g. std_typanalyze) can set it to negative
value. This is the same check as in examine_attribute, and we need the
same protections I think.
> For serialize_expr_stats():
>
> + sd = table_open(StatisticRelationId, RowExclusiveLock);
> +
> + /* lookup OID of composite type for pg_statistic */
> + typOid = get_rel_type_id(StatisticRelationId);
> + if (!OidIsValid(typOid))
> + ereport(ERROR,
>
> Looks like the table_open() call can be made after the typOid check.
>
Probably, but this failure is unlikely (should never happen) so I don't
think this makes any real difference.
> + Datum values[Natts_pg_statistic];
> + bool nulls[Natts_pg_statistic];
> + HeapTuple stup;
> +
> + if (!stats->stats_valid)
>
> It seems the local arrays can be declared after the validity check.
>
Nope, that would be against C99.
> + if (enabled[i] == STATS_EXT_NDISTINCT)
> + ndistinct_enabled = true;
> + if (enabled[i] == STATS_EXT_DEPENDENCIES)
> + dependencies_enabled = true;
> + if (enabled[i] == STATS_EXT_MCV)
>
> the second and third if should be preceded with 'else'
>
Yeah, although this just moves existing code. But you're right it could
use else.
> +ReleaseDummy(HeapTuple tuple)
> +{
> + pfree(tuple);
>
> Since ReleaseDummy() is just a single pfree call, maybe you don't need
> this method - call pfree in its place.
>
No, that's not possible because the freefunc callback expects signature
void (*)(HeapTupleData *)
and pfree() does not match that.
thanks
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@enterprisedb.com, zyu@yugabyte.com, pryzby@telsasoft.com, dean.a.rasheed@gmail.com
Subject: Re: PoC/WIP: Extended statistics on expressions
In-Reply-To: <01a2e09f-843a-824b-42c4-4d31d173b4a7@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox