Обсуждение: Report roles in pg_upgrade pg_ prefix check
Looking at a recent pg_upgrade thread I happened to notice that the check for roles with a pg_ prefix only reports the error, not the roles it found. Other similar checks where the user is expected to alter the old cluster typically reports the found objects in a textfile. The attached adds reporting to make that class of checks consistent (the check for prepared transactions which also isn't reporting is different IMO as it doesn't expect ALTER commands). As this check is only executed against the old cluster the patch removes the check when printing the error. -- Daniel Gustafsson https://vmware.com/
Вложения
On Thu, Nov 24, 2022 at 12:31:09PM +0100, Daniel Gustafsson wrote: > Looking at a recent pg_upgrade thread I happened to notice that the check for > roles with a pg_ prefix only reports the error, not the roles it found. Other > similar checks where the user is expected to alter the old cluster typically > reports the found objects in a textfile. The attached adds reporting to make > that class of checks consistent (the check for prepared transactions which also > isn't reporting is different IMO as it doesn't expect ALTER commands). > > As this check is only executed against the old cluster the patch removes the > check when printing the error. +1 -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Embrace your flaws. They make you human, rather than perfect, which you will never be.
On Thu, Nov 24, 2022 at 12:31:09PM +0100, Daniel Gustafsson wrote: > Looking at a recent pg_upgrade thread I happened to notice that the check for > roles with a pg_ prefix only reports the error, not the roles it found. Other > similar checks where the user is expected to alter the old cluster typically > reports the found objects in a textfile. The attached adds reporting to make > that class of checks consistent (the check for prepared transactions which also > isn't reporting is different IMO as it doesn't expect ALTER commands). > > As this check is only executed against the old cluster the patch removes the > check when printing the error. +1. A backpatch would be nice, though not strictly mandatory as that's not a bug fix. + ntups = PQntuples(res); + i_rolname = PQfnumber(res, "rolname"); Would it be worth adding the OID on top of the role name in the generated report? That would be a free meal. -- Michael
Вложения
> On 28 Nov 2022, at 02:18, Michael Paquier <michael@paquier.xyz> wrote: > > On Thu, Nov 24, 2022 at 12:31:09PM +0100, Daniel Gustafsson wrote: >> Looking at a recent pg_upgrade thread I happened to notice that the check for >> roles with a pg_ prefix only reports the error, not the roles it found. Other >> similar checks where the user is expected to alter the old cluster typically >> reports the found objects in a textfile. The attached adds reporting to make >> that class of checks consistent (the check for prepared transactions which also >> isn't reporting is different IMO as it doesn't expect ALTER commands). >> >> As this check is only executed against the old cluster the patch removes the >> check when printing the error. > > +1. A backpatch would be nice, though not strictly mandatory as > that's not a bug fix. Yeah, it doesn't really qualify since this not a bugfix. > + ntups = PQntuples(res); > + i_rolname = PQfnumber(res, "rolname"); > > Would it be worth adding the OID on top of the role name in the > generated report? That would be a free meal. We are a bit inconsistent in how much details we include in the report textfiles, so could do that without breaking any consistency in reporting. Looking at other checks, the below format would match what we already do fairly well: <rolname> (oid=<oid>) Done in the attached. -- Daniel Gustafsson https://vmware.com/
Вложения
On Mon, Nov 28, 2022 at 09:58:46AM +0100, Daniel Gustafsson wrote: > We are a bit inconsistent in how much details we include in the report > textfiles, so could do that without breaking any consistency in reporting. > Looking at other checks, the below format would match what we already do fairly > well: > > <rolname> (oid=<oid>) > > Done in the attached. WFM. Thanks! -- Michael
Вложения
> On 29 Nov 2022, at 00:24, Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Nov 28, 2022 at 09:58:46AM +0100, Daniel Gustafsson wrote: >> We are a bit inconsistent in how much details we include in the report >> textfiles, so could do that without breaking any consistency in reporting. >> Looking at other checks, the below format would match what we already do fairly >> well: >> >> <rolname> (oid=<oid>) >> >> Done in the attached. > > WFM. Thanks! Took another look at it, and applied it. Thanks! -- Daniel Gustafsson https://vmware.com/