Re: Schema variables - new implementation for Postgres 15

Hi,

On Sat, Nov 05, 2022 at 05:04:31PM +0100, Tomas Vondra wrote:
>
> I did a quick initial review of this patch series - attached is a
> version with "review" commits for some of the parts. The current patch
> seems in pretty good shape, most of what I noticed are minor issues. I
> plan to do a more thorough review later.

Thanks!

I agree with all of your comments, just a few answers below

> - NamesFromList and IdentifyVariable seem introduced unnecessarily
> early, as they are only used in 0002 and 0003 parts (in the original
> patch series). Not sure if the plan is to squash everything into a
> single patch, or commit individual patches.

The split was mostly done to make the patch easier to review, as it adds quite
a bit of infrastructure.

There have been some previous comments to have a more logical separation and
fix similar issues, but there are still probably other oddities like that
laying around. I personally didn't focus much on it as I don't know if the
future committer will choose to squash everything or not.

> - AFAIK patches don't need to modify typedefs.list.

I think this was discussed a year or so ago, and my understanding is that the
general rule is that it's now welcome, if not recommended, to maintain
typedefs.list in each patchset.

> Which I think means this:
>
> if (filter_lxid && svar->drop_lxid == MyProc->lxid)
> continue;
>
> accesses drop_lxid, which was not initialized in init_session_variable.

Agreed.