Обсуждение: [COMMITTERS] pgsql: Assert that we don't invent relfilenodes or type OIDs inbinary
Assert that we don't invent relfilenodes or type OIDs in binary upgrade. During pg_upgrade's restore run, all relfilenode choices should be overridden by commands in the dump script. If we ever find ourselves choosing a relfilenode in the ordinary way, someone blew it. Likewise for pg_type OIDs. Since pg_upgrade might well succeed anyway, if there happens not to be a conflict during the regression test run, we need assertions here to keep us on the straight and narrow. We might someday be able to remove the assertion in GetNewRelFileNode, if pg_upgrade is rewritten to remove its assumption that old and new relfilenodes always match. But it's hard to see how to get rid of the pg_type OID constraint, since those OIDs are embedded in user tables in some cases. Back-patch as far as 9.5, because of the risk of back-patches breaking something here even if it works in HEAD. I'd prefer to go back further, but 9.4 fails both assertions due to get_rel_infos()'s use of a temporary table. We can't use the later-branch solution of a CTE for compatibility reasons (cf commit 5d16332e9), and it doesn't seem worth inventing some other way to do the query. (I did check, by dint of changing the Asserts to elog(WARNING), that there are no other cases of unwanted OID assignments during 9.4's regression test run.) Discussion: https://postgr.es/m/19785.1497215827@sss.pgh.pa.us Branch ------ REL9_6_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/318fd99ce7e775158c87b8515780f2663d2df429 Modified Files -------------- src/backend/catalog/catalog.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
Re: [COMMITTERS] pgsql: Assert that we don't invent relfilenodes ortype OIDs in binary
От
Bruce Momjian
Дата:
Uh, is there a reason this is only an Assert(), meaning it only checks in assert builds. pg_upgrade already has a lot of checks and they are all fatal. --------------------------------------------------------------------------- On Tue, Jun 13, 2017 at 12:04:48AM +0000, Tom Lane wrote: > Assert that we don't invent relfilenodes or type OIDs in binary upgrade. > > During pg_upgrade's restore run, all relfilenode choices should be > overridden by commands in the dump script. If we ever find ourselves > choosing a relfilenode in the ordinary way, someone blew it. Likewise for > pg_type OIDs. Since pg_upgrade might well succeed anyway, if there happens > not to be a conflict during the regression test run, we need assertions > here to keep us on the straight and narrow. > > We might someday be able to remove the assertion in GetNewRelFileNode, > if pg_upgrade is rewritten to remove its assumption that old and new > relfilenodes always match. But it's hard to see how to get rid of the > pg_type OID constraint, since those OIDs are embedded in user tables > in some cases. > > Back-patch as far as 9.5, because of the risk of back-patches breaking > something here even if it works in HEAD. I'd prefer to go back further, > but 9.4 fails both assertions due to get_rel_infos()'s use of a temporary > table. We can't use the later-branch solution of a CTE for compatibility > reasons (cf commit 5d16332e9), and it doesn't seem worth inventing some > other way to do the query. (I did check, by dint of changing the Asserts > to elog(WARNING), that there are no other cases of unwanted OID assignments > during 9.4's regression test run.) > > Discussion: https://postgr.es/m/19785.1497215827@sss.pgh.pa.us > > Branch > ------ > REL9_6_STABLE > > Details > ------- > https://git.postgresql.org/pg/commitdiff/318fd99ce7e775158c87b8515780f2663d2df429 > > Modified Files > -------------- > src/backend/catalog/catalog.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > > -- > Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-committers -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Bruce Momjian <bruce@momjian.us> writes:
> Uh, is there a reason this is only an Assert(), meaning it only checks
> in assert builds. pg_upgrade already has a lot of checks and they are
> all fatal.
Yeah, I didn't think it was worth adding overhead to production builds
for it.
regards, tom lane
Re: [COMMITTERS] pgsql: Assert that we don't invent relfilenodes ortype OIDs in binary
От
Bruce Momjian
Дата:
On Tue, Jun 13, 2017 at 01:39:34PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Uh, is there a reason this is only an Assert(), meaning it only checks > > in assert builds. pg_upgrade already has a lot of checks and they are > > all fatal. > > Yeah, I didn't think it was worth adding overhead to production builds > for it. Uh, you realize there are already many pg_upgrade sanity checks in the backend that are not asserts, right? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: [COMMITTERS] pgsql: Assert that we don't invent relfilenodes ortype OIDs in binary
От
Andres Freund
Дата:
On 2017-06-13 13:45:12 -0400, Bruce Momjian wrote: > On Tue, Jun 13, 2017 at 01:39:34PM -0400, Tom Lane wrote: > > Bruce Momjian <bruce@momjian.us> writes: > > > Uh, is there a reason this is only an Assert(), meaning it only checks > > > in assert builds. pg_upgrade already has a lot of checks and they are > > > all fatal. > > > > Yeah, I didn't think it was worth adding overhead to production builds > > for it. > > Uh, you realize there are already many pg_upgrade sanity checks in the > backend that are not asserts, right? And? Not that it'll make a huge difference, but GetNewOidWithIndex() is a relatively hot-path in some workloads (e.g. with lots of toasted data), so it actually can make a difference. And for debugging asserts are often actually more useful, because you get a backtrace. I'm not sure what you're actually concerned about here? - Andres
Re: [COMMITTERS] pgsql: Assert that we don't invent relfilenodes ortype OIDs in binary
От
Bruce Momjian
Дата:
On Tue, Jun 13, 2017 at 11:52:04AM -0700, Andres Freund wrote: > On 2017-06-13 13:45:12 -0400, Bruce Momjian wrote: > > On Tue, Jun 13, 2017 at 01:39:34PM -0400, Tom Lane wrote: > > > Bruce Momjian <bruce@momjian.us> writes: > > > > Uh, is there a reason this is only an Assert(), meaning it only checks > > > > in assert builds. pg_upgrade already has a lot of checks and they are > > > > all fatal. > > > > > > Yeah, I didn't think it was worth adding overhead to production builds > > > for it. > > > > Uh, you realize there are already many pg_upgrade sanity checks in the > > backend that are not asserts, right? > > And? Not that it'll make a huge difference, but GetNewOidWithIndex() is > a relatively hot-path in some workloads (e.g. with lots of toasted > data), so it actually can make a difference. And for debugging asserts > are often actually more useful, because you get a backtrace. > > I'm not sure what you're actually concerned about here? I am concerned a non-assert build will not error out, but if no one else is concerned about that, I am fine. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Bruce Momjian <bruce@momjian.us> writes:
> On Tue, Jun 13, 2017 at 11:52:04AM -0700, Andres Freund wrote:
>> I'm not sure what you're actually concerned about here?
> I am concerned a non-assert build will not error out, but if no one else
> is concerned about that, I am fine.
I think in a production situation, we actually don't want it to error
out. The odds are fairly good that the run would complete successfully
(ie, the potential OID collision never actually materializes). So all
we're doing is converting a possible failure into an unavoidable one.
Where we want to hear about the problem is in development. So really
an Assert is the right thing.
regards, tom lane
Re: [COMMITTERS] pgsql: Assert that we don't invent relfilenodes ortype OIDs in binary
От
Bruce Momjian
Дата:
On Tue, Jun 13, 2017 at 03:10:16PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Tue, Jun 13, 2017 at 11:52:04AM -0700, Andres Freund wrote: > >> I'm not sure what you're actually concerned about here? > > > I am concerned a non-assert build will not error out, but if no one else > > is concerned about that, I am fine. > > I think in a production situation, we actually don't want it to error > out. The odds are fairly good that the run would complete successfully > (ie, the potential OID collision never actually materializes). So all > we're doing is converting a possible failure into an unavoidable one. > > Where we want to hear about the problem is in development. So really > an Assert is the right thing. OK, but my point is that all the other places, which seems similar, error out in production. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +