Hi,
On Tue, Mar 22, 2022 at 03:21:20PM +0300, Aleksander Alekseev wrote:
>
> The v3-0001 patch LGTM.
>
> Since v3-0002 adds a new view and alters pg_proc.dat shouldn't it also
> increase CATALOG_VERSION_NO? Not sure if we generally do this in the
> patches or expect the committer to make the change manually.
It's better to no include any catversion bump, otherwise the patches will
rot very fast. Author can mention the need for a catversion bump in the
commit message, and I usually do so but I apparently forgot.
> Same question regarding v3-0003. Other than that the patch looks OK, but
> doesn't seem to add any tests for the new functionality. Do you think it
> would be possible to test-cover the file inclusion? Personally I don't
> think it's that critical to have these particular tests, but if they can be
> added, I think we should do so.
Yes, as I mentioned in the first email I'm willing to add test coverage. But
this will require TAP tests, and it's likely going to be a bit annoying to make
sure it has decent coverage and works on all platforms, so I'd rather do it
once I know there's at least some basic agreement on the feature and/or the
approach. Unfortunately, until now there was no feedback on that part despite
the activity on the thread. In my experience it's a good sign that this will
get rejected soon, so I still didn't write tests.
> I didn't review v3-0004 since it's marked as PoC and seems to be a separate
> feature that is not targeting PG15. I suggest excluding it from the
> patchset in order to keep the focus. Consider creating a new thread and a
> new CF entry after we deal with v3-0001...v3-0003.
This feature was discussed in some old thread when the file inclusion was first
discussed almost a decade ago, and in my understanding it was part of the "must
have" (along with 0002) in order to accept the feature. That's why I added
it, since I'm also willing to work of that if needed, whether before or after
the file inclusion thing.
> All in all, the patchset seems to be in good shape and I don't have
> anything but some little nitpicks. It passes `make installcheck` and I
> verified manually that the file inclusion 1) works 2) write proper error
> messages to the logfile when the included file doesn't exist or has wrong
> permissions.
Thanks!
