Re: plan shape work

On Mon, Sep 29, 2025 at 11:12 PM Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Sep 29, 2025 at 4:52 AM Richard Guo <guofenglinux@gmail.com> wrote:
> > It seems no one has attempted to code up the approach I suggested, so
> > I went ahead and did it; please see the attached PoC patch.  It's just
> > a proof of concept to show what I have in mind, so please excuse the
> > lack of comments and necessary assertions for now.

> I think that if there are subqueries named expr_1 and expr_3, this
> will assign the name expr_4 next, whereas the previous patch assigns
> the name expr_2. That might not be a bug, but it's different and I
> don't like it as well.
>
> I also think that if there are subqueries named expr_1a and expr_2a,
> this will assign the name expr_3 next, whereas the previous patch
> assigns the name expr_1. I would consider that a clear bug.
>
> Your code will also see expr_01 and decide that the next name should
> be expr_2 rather than expr_1. I don't think that's right, either.
>
> I also think that it's a bug that your function sometimes returns a
> value different from the one it appends to canon_names.

I don't think any of the bugs you described upthread exist in the PoC
patch.  The patch ensures that all names stored in glob->subplanNames
are canonicalized to the format "$basename_$suffixnum".  If no numeric
suffix is required, the name is stored with a "_0" suffix.  This
guarantees a clear distinction between the base name and the numeric
suffix for all names stored in the list.  (Please note that this
canonicalization applies only to how names are stored internally, not
to user-visible names.  So no user-visible behavior change here.)

I don't really see how uncanonicalized names like expr_1a, expr_2a, or
expr_01 would appear in the subplanNames list to begin with.  Perhaps
you're referring to user-supplied subquery aliases?  In that case, the
patch deliberately avoids matching expr to those names, since it only
compares the base name.  As a result, it would assign expr_1 as the
next name, which is the expected behavior.

This PoC patch passes all the regression tests, which at least, IMHO,
suggests that it avoids such basic bugs.

However, since both you and Tom feel this proposal doesn't make
sense, I'll withdraw it.  Apologies for any trouble this has caused.

- Richard