public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alena Rybakina <[email protected]>
To: Yuya Watari <[email protected]>
Cc: Andrei Lepikhov <[email protected]>
Cc: Ashutosh Bapat <[email protected]>
Cc: David Rowley <[email protected]>
Cc: Thom Brown <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Zhang Mingli <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: PostgreSQL Developers <[email protected]>
Subject: Re: [PoC] Reducing planning time when tables have many partitions
Date: Tue, 13 Feb 2024 00:19:21 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAJ2pMkaaY4FgGSO2z9X_XOoqtLMUhyx6cPZFQbxmsjLgLpEd2Q@mail.gmail.com>
References: <CAJ2pMkZwp545Fj=vyiyy=j=zPAEisb1=72PAU6vwyNc=Nx9vpA@mail.gmail.com>
<CAJ2pMkaN57du6Yw+oomt44Ru+P2Hdp+vigexeYqUtbwyy=8SGA@mail.gmail.com>
<CAApHDvqp0UrCf9FLGyPWruiuU6BfPPquDxE2Wxw7ZS2S1FNH0A@mail.gmail.com>
<CAJ2pMka06YYV9UYx+NT1zkJO0ah+KHYWJ2vOmV2qGphfU5TjeQ@mail.gmail.com>
<CAJ2pMkZk-Nr=yCKrGfGLu35gK-D179QPyxaqtJMUkO86y1NmSA@mail.gmail.com>
<[email protected]>
<CAJ2pMkaNzmvMUm9igQwRH0AAo39gsjnE1VXupPGyLR2T7ENnUQ@mail.gmail.com>
<[email protected]>
<CAJ2pMkbdVPqQcJ=vn4e7T+SmMGUVKvU2OcYZHFLD-paWEbaGZg@mail.gmail.com>
<[email protected]>
<CAJ2pMkbf+eWNEBTMdue03We2JtaQLvye7OsZB-v_EJmFQHo_gw@mail.gmail.com>
<CAJ2pMkYgCc=JsyQjMMKxVuSi1g6Ny8YZS6YSLsXrebA4mwy0OQ@mail.gmail.com>
<CAJ2pMkZGB4Yx1dCYkU_YRJgj2rcC0s+PWknqrszmsRPHGLqCgg@mail.gmail.com>
<[email protected]>
<CAJ2pMkaaY4FgGSO2z9X_XOoqtLMUhyx6cPZFQbxmsjLgLpEd2Q@mail.gmail.com>
Hi! Sorry my delayed reply too.
On 17.01.2024 12:33, Yuya Watari wrote:
> Hello Alena,
>
> Thank you for your quick response, and I'm sorry for my delayed reply.
>
> On Sun, Dec 17, 2023 at 12:41 AM Alena Rybakina
> <[email protected]> wrote:
>> I thought about this earlier and was worried that the index links of the equivalence classes might not be referenced correctly for outer joins,
>> so I decided to just overwrite them and reset the previous ones.
> Thank you for pointing this out. I have investigated this problem and
> found a potential bug place. The code quoted below modifies
> RestrictInfo's clause_relids. Here, our indexes, namely
> eclass_source_indexes and eclass_derive_indexes, are based on
> clause_relids, so they should be adjusted after the modification.
> However, my patch didn't do that, so it may have missed some
> references. The same problem occurs in places other than the quoted
> one.
>
> =====
> /*
> * Walker function for replace_varno()
> */
> static bool
> replace_varno_walker(Node *node, ReplaceVarnoContext *ctx)
> {
> ...
> else if (IsA(node, RestrictInfo))
> {
> RestrictInfo *rinfo = (RestrictInfo *) node;
> ...
>
> if (bms_is_member(ctx->from, rinfo->clause_relids))
> {
> replace_varno((Node *) rinfo->clause, ctx->from, ctx->to);
> replace_varno((Node *) rinfo->orclause, ctx->from, ctx->to);
> rinfo->clause_relids = replace_relid(rinfo->clause_relids,
> ctx->from, ctx->to);
> ...
> }
> ...
> }
> ...
> }
> =====
>
> I have attached a new version of the patch, v23, to fix this problem.
> v23-0006 adds a helper function called update_clause_relids(). This
> function modifies RestrictInfo->clause_relids while adjusting its
> related indexes. I have also attached a sanity check patch
> (sanity-check.txt) to this email. This sanity check patch verifies
> that there are no missing references between RestrictInfos and our
> indexes. I don't intend to commit this patch, but it helps find
> potential bugs. v23 passes this sanity check, but the v21 you
> submitted before does not. This means that the adjustment by
> update_clause_relids() is needed to prevent missing references after
> modifying clause_relids. I'd appreciate your letting me know if v23
> doesn't solve your concern.
>
> One of the things I don't think is good about my approach is that it
> adds some complexity to the code. In my approach, all modifications to
> clause_relids must be done through the update_clause_relids()
> function, but enforcing this rule is not so easy. In this sense, my
> patch may need to be simplified more.
>
>> this is due to the fact that I explained before: we zeroed the values indicated by the indexes,
>> then this check is not correct either - since the zeroed value indicated by the index is correct.
>> That's why I removed this check.
> Thank you for letting me know. I fixed this in v23-0005 to adjust the
> indexes in update_eclasses(). With this change, the assertion check
> will be correct.
>
Yes, it is working correctly now with the assertion check. I suppose
it's better to add this code with an additional comment and a
recommendation for other developers
to use it for checking in case of manipulations with the list of
equivalences.
--
Regards,
Alena Rybakina
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
view thread (21+ messages) latest in thread
reply
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PoC] Reducing planning time when tables have many partitions
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox