Thread: Tables created WITH OIDS cannot be dumped/restored properly

PG version: 11.0
OS: Ubuntu 16.04 (also verified on Ubuntu 14.04 and 18.04)
Package: Binaries installed via official PostgreSQL apt repository

--

Hello PG community! I'm one of the PipelineDB developers (PostgreSQL 
extension) and as we've begun adding support for PG 11 our test 
infrastructure identified an issue with dumping/restoring tables created 
WITH OIDs. Naturally this also affects pg_upgrade.

If a table is created WITH OIDS, dumped and then restored, the resulting 
table will not contain the OID column.

I have attached a short script that reproduces the issue reliably (at 
least on the aforementioned operating systems), and I'll briefly outline 
the steps I took here:

1) Initialize data directory and run server
2) psql -c "CREATE TABLE test (x integer) WITH OIDS"
3) psql -c "SELECT oid FROM test" -> Ok, column is there
4) pg_dump > dump.sql
5) Stop server
6) Initialize new data directory and run server
7) psql -f dump.sql
8) psql -c "SELECT oid FROM test" -> ERROR:  column "oid" does not exist

This behavior appears to have been introduced by this commit:

https://github.com/postgres/postgres/commit/9a95a77d9d5d3003d2d67121f

In pg_backup_archiver.c, there is a comparison being done between two 
bool fields to determine whether or not to output "SET default_with_oids 
= on" before a table's CREATE TABLE dump output:

https://github.com/postgres/postgres/blob/master/src/bin/pg_dump/pg_backup_archiver.c#L3385

And it appears that the above commit changes the bool type from char to 
_Bool (assuming stdbool.h is present) which has led to unexpected 
results for this comparison. These two bool fields are not only being 
set to true and false--they're being set to true, false, and "not set" 
(signified by -1). I believe the -1 assignment does not behave as 
expected when bool is _Bool.

Ultimately this prevents pg_backup_archiver.c from outputting "SET 
default_with_oids = on" prior to the dumped CREATE TABLE command, and 
thus the table is created without OIDs.

--

The simplest fix for the issue is to revert the types of these two bool 
fields to char, as prior to this commit bool would have been defined as 
char. Perhaps a more involved solution is warranted, but this was the 
least intrusive way I saw to restore the behavior from prior to the 
introduction of stdbool.h.

I've attached a patch containing this change, and would be happy to 
submit it elsewhere if necessary or improve upon it based on any 
guidance from the community. Thank you!

On 01/11/2018 20:56, Derek Nelson wrote:
> Hello PG community! I'm one of the PipelineDB developers (PostgreSQL 
> extension) and as we've begun adding support for PG 11 our test 
> infrastructure identified an issue with dumping/restoring tables created 
> WITH OIDs. Naturally this also affects pg_upgrade.
> 
> If a table is created WITH OIDS, dumped and then restored, the resulting 
> table will not contain the OID column.

Good catch.  It's actually only if the affected table is the first table
in the output (or perhaps some other narrow circumstances).  We do have
a test case for WITH OIDS tables, but that table ends up not the first
in the output.

I propose to apply the attached patch.  It's slightly different from
yours in that I don't think we need to change the type of the withOids
field.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 12/11/2018 21:00, Derek Nelson wrote:
> Thanks for the quick turnaround! I considered only changing the type of
> currWithOids but thought it may make sense to change both for uniformity
> as well as future proofing against a similar issue being introduced
> again moving forward. In any case, this seems like a good, minimal
> fix--thanks again!

Committed to master and PG11.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 15/11/2018 04:27, Derek Nelson wrote:
> Thank you so much Peter! Is this backported to the official PG11
> releases or will it become available when 11.x is out?

I'm not quite sure what the difference between these two statements is.
The fix will be in PG 11.2 in any case.

By the way, we are currently planning on getting rid of WITH OIDS
support altogether in PG 12.

> On Tue, Nov 13, 2018, 12:54 AM Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com
> <mailto:peter.eisentraut@2ndquadrant.com> wrote:
> 
>     On 12/11/2018 21:00, Derek Nelson wrote:
>     > Thanks for the quick turnaround! I considered only changing the
>     type of
>     > currWithOids but thought it may make sense to change both for
>     uniformity
>     > as well as future proofing against a similar issue being introduced
>     > again moving forward. In any case, this seems like a good, minimal
>     > fix--thanks again!
> 
>     Committed to master and PG11.
> 
>     -- 
>     Peter Eisentraut              http://www.2ndQuadrant.com/
>     PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> 


-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services