Re: [PoC] Reducing planning time when tables have many partitions

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
> <lena.ribackina@yandex.ru> wrote:
>> I thought about this earlier and was worried that the index links of the equivalence classes might not be referenced
correctlyfor 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