On Wed, 2024-05-22 at 19:22 +0200, Peter Eisentraut wrote: > On 29.04.24 10:23, Peter Eisentraut wrote: > > Here is a patch set to implement virtual generated columns. > > > The main feature patch (0005 here) generally works but has a number > > of > > open corner cases that need to be thought about and/or fixed, many > > of > > which are marked in the code or the tests. I'll continue working > > on > > that. But I wanted to see if I can get some feedback on the test > > structure, so I don't have to keep changing it around later. > > Here is an updated patch set. It needed some rebasing, especially > around the reverting of the catalogued not-null constraints. I have > also fixed up the various incomplete or "fixme" pieces of code > mentioned > above. I have in most cases added "not supported yet" error messages > for now, with the idea that some of these things can be added in > later, > as incremental features. >
This is not (yet) full review. Patches applied cleanly on 76618097a6c027ec603a3dd143f61098e3fb9794 from 2024-06-14. I've run ./configure && make world-bin && make check && make check-world on 0001, then 0001+0002, then 0001+0002+0003, up to applying all 5 patches. All cases passed on Debian unstable on aarch64 (ARM64) on gcc (Debian 13.2.0-25) 13.2.0. v1-0001-Rename-regress-test-generated-to-generated_stored.patch: no objections here, makes sense as preparation for future changes v1-0002-Put-generated_stored-test-objects-in-a-schema.patch: also no objections. OTOH other tests (like publication.out, rowsecurity.out, stats_ext.out, tablespace.out) are creating schemas and later dropping them - so here it might also make sense to drop schema at the end of testing. v1-0003-Remove-useless-initializations.patch: All other cases (I checked directory src/backend/utils/cache) calling MemoryContextAllocZero only initialize fields when they are non-zero, so removing partial initialization with false brings consistency to the code. v1-0004-Remove-useless-code.patch: Patch removes filling in of constraints from function BuildDescForRelation. This function is only called from file view.c and tablecmds.c (twice). In none of those cases result->constr is used, so proposed change makes sense. While I do not know code well, so might be wrong here, I would apply this patch. I haven't looked at the most important (and biggest) file yet, v1-0005-Virtual-generated-columns.patch; hope to look at it this week. At the same I believe 0001-0004 can be applied - even backported if it'll make maintenance of future changes easier. But that should be commiter's decision. Best regards -- Tomasz Rybak, Debian Developer <serp...@debian.org> GPG: A565 CE64 F866 A258 4DDC F9C7 ECB7 3E37 E887 AA8C
signature.asc
Description: This is a digitally signed message part
