On Wed, Feb 22, 2023 at 10:37:35AM +0100, Daniel Gustafsson wrote: >> On 18 Feb 2023, at 06:46, Nathan Bossart <nathandboss...@gmail.com> wrote: >> The last 4 are for supported versions and, from a very >> quick glance, seem possible to consolidate. That would bring us to a total >> of 11 separate loops that we could consolidate into one. However, the data >> type checks seem to follow a nice pattern, so perhaps this is easier said >> than done. > > There is that, refactoring the data type checks leads to removal of duplicated > code and a slight performance improvement. Refactoring the other checks to > reduce overhead would be an interesting thing to look at, but this point in > the > v16 cycle might not be ideal for that.
Makes sense. >> I wonder if it'd be better to perform all of the data type >> checks in all databases before failing so that all of the violations are >> reported. Else, users would have to run pg_upgrade, fix a violation, run >> pg_upgrade again, fix another one, etc. > > I think that's better, and have changed the patch to do it that way. Thanks. This seems to work as intended. One thing I noticed is that the "failed check" log is only printed once, even if multiple data type checks failed. I believe this is because this message uses PG_STATUS. If I change it to PG_REPORT, all of the "failed check" messages appear. TBH I'm not sure we need this message at all since a more detailed explanation will be printed afterwards. If we do keep it around, I think it should be indented so that it looks more like this: Checking for data type usage checking all databases failed check: incompatible aclitem data type in user tables failed check: reg* data types in user tables > One change this brings is that check.c contains version specific checks in the > struct. Previously these were mostly contained in version.c (some, like the > 9.4 jsonb check was in check.c) which maintained some level of separation. > Splitting the array init is of course one option but it also seems a tad > messy. > Not sure what's best, but for now I've documented it in the array comment at > least. Hm. We could move check_for_aclitem_data_type_usage() and check_for_jsonb_9_4_usage() to version.c since those are only used for determining whether the check applies now. Otherwise, IMO things are in roughly the right place. I don't think it's necessary to split the array. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com