Fast default stuff versus pg_upgrade

AFAICS, the fast-default patch neglected to consider what happens if
a database containing columns with active attmissingval entries is
pg_upgraded.  I do not see any code in either pg_dump or pg_upgrade that
attempts to deal with that situation, which means the effect will be
that the "missing" values will silently revert to nulls: they're still
null in the table storage, and the restored pg_attribute entries won't
have anything saying it should be different.

The pg_upgrade regression test fails to exercise such a case.  There is
only one table in the ending state of the regression database that has
any atthasmissing columns, and it's empty :-(.  If I add a table in
which there actually are active attmissingval entries, say according
to the attached patch, I get a failure in the pg_upgrade test.

This is certainly a stop-ship issue, and in fact it's bad enough
that I think we may need to pull the feature for v11.  Designing
binary-upgrade support for this seems like a rather large task
to be starting post-beta1.  Nor do I think it's okay to wait for
v12 to make it work; what if we have to force an initdb later in
beta, or recommend use of pg_upgrade for some manual catalog fix
after release?

                        regards, tom lane


diff --git a/src/test/regress/expected/fast_default.out b/src/test/regress/expected/fast_default.out
index ef8d04f..f3d783c 100644
*** a/src/test/regress/expected/fast_default.out
--- b/src/test/regress/expected/fast_default.out
*************** DROP TABLE has_volatile;
*** 548,550 ****
--- 548,561 ----
  DROP EVENT TRIGGER has_volatile_rewrite;
  DROP FUNCTION log_rewrite;
  DROP SCHEMA fast_default;
+ -- Leave a table with an active fast default in place, for pg_upgrade testing
+ set search_path = public;
+ create table has_fast_default(f1 int);
+ insert into has_fast_default values(1);
+ alter table has_fast_default add column f2 int default 42;
+ table has_fast_default;
+  f1 | f2
+ ----+----
+   1 | 42
+ (1 row)
+
diff --git a/src/test/regress/sql/fast_default.sql b/src/test/regress/sql/fast_default.sql
index 0e66033..7b9cc47 100644
*** a/src/test/regress/sql/fast_default.sql
--- b/src/test/regress/sql/fast_default.sql
*************** DROP TABLE has_volatile;
*** 369,371 ****
--- 369,378 ----
  DROP EVENT TRIGGER has_volatile_rewrite;
  DROP FUNCTION log_rewrite;
  DROP SCHEMA fast_default;
+
+ -- Leave a table with an active fast default in place, for pg_upgrade testing
+ set search_path = public;
+ create table has_fast_default(f1 int);
+ insert into has_fast_default values(1);
+ alter table has_fast_default add column f2 int default 42;
+ table has_fast_default;

On 06/19/2018 10:55 AM, Tom Lane wrote:

Ouch!

I guess I have to say mea culpa.

My initial thought was that as a fallback we should disable pg_upgrade
on databases containing such values, and document the limitation in the
docs and the release notes. The workaround would be to force a table
rewrite which would clear them if necessary.

Have we ever recommended use of pg_upgrade for some manual catalog fix
after release? I don't recall doing so. Certainly it hasn't been common.

I have no idea how large an actual fix might be. I'll at least start
working on it immediately. I agree it's very late in the day.

cheers

andrew

Hi,

On 2018-06-19 11:51:16 -0400, Andrew Dunstan wrote:
> My initial thought was that as a fallback we should disable pg_upgrade on
> databases containing such values, and document the limitation in the docs
> and the release notes. The workaround would be to force a table rewrite
> which would clear them if necessary.

I personally would say that that's not acceptable. People will start
using fast defaults - and you can't even do anything against it! - and
suddenly pg_upgrade won't work. But they will only notice that years
later, after collecting terrabytes of data in such tables.

If we can't fix it properly, then imo we should revert / neuter the
feature.


> Have we ever recommended use of pg_upgrade for some manual catalog fix after
> release? I don't recall doing so. Certainly it hasn't been common.

No, but why does it matter? Are you arguing we can delay pg_dump support
for fast defaults to v12?

Greetings,

Andres Freund

Andres Freund <[hidden email]> writes:
> On 2018-06-19 11:51:16 -0400, Andrew Dunstan wrote:
>> Have we ever recommended use of pg_upgrade for some manual catalog fix after
>> release? I don't recall doing so. Certainly it hasn't been common.

> No, but why does it matter?

We absolutely have, as recently as last month:

        * Fix incorrect volatility markings on a few built-in functions
          (Thomas Munro, Tom Lane)

        ... can be fixed by manually updating these functions' pg_proc
        entries, for example ALTER FUNCTION pg_catalog.query_to_xml(text,
        boolean, boolean, text) VOLATILE. (Note that that will need to be
        done in each database of the installation.) Another option is to
        pg_upgrade the database to a version containing the corrected
        initial data.

                        regards, tom lane

Andrew Dunstan <[hidden email]> writes:
> I have no idea how large an actual fix might be. I'll at least start
> working on it immediately. I agree it's very late in the day.

On reflection, it seems like there are two moving parts needed:

* Add a binary-upgrade support function to the backend, which would take,
say, table oid, column name, and some representation of the default value;

* Teach pg_dump when operating in binary-upgrade mode to emit a call to
such a function for each column that has atthasmissing true.

The hard part here is how exactly are we going to represent the default
value.  AFAICS, the only thing that pg_dump could readily lay its hands
on is the "anyarray" textual representation of attmissingval, which maybe
is okay but it means more work for the support function.  Too bad we did
not just store the value in bytea format.

                        regards, tom lane

On 06/19/2018 12:05 PM, Andres Freund wrote:

Umm, barring the case that Tom mentioned by then it would just work.
It's not the case that if they put in fast default values today they
will never be able to upgrade.



Right now I'm more or less thinking out loud, not arguing anything.

I'd at least like to see what a solution might look like before ruling
it out. I suspect I can come up with something in a day or so. The work
wouldn't be wasted.

cheers

andrew

--
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-06-19 12:17:56 -0400, Tom Lane wrote:
Isn't that just a few lines of code? And if the default value bugs us,
we can easily add a support function that dumps the value without the
anyarray adornment?


> Too bad we did not just store the value in bytea format.

That still seems the right thing to me, not being able in areasonable
way to inspect the default values in the catalog seems worse.  We could
have added a new non-array pseudo-type as well, but that's a fair bit of
work...

Greetings,

Andres Freund

On 2018-06-19 12:17:59 -0400, Andrew Dunstan wrote:
Huh?


> It's not the case that if they put in fast default values today they
> will never be able to upgrade.

How? I mean upgrading and loosing your default values certainly ain't
ok?  And we can't expect users to rewrite their tables, that's why we
added fast default support and why pg_upgrade is used.


> I'd at least like to see what a solution might look like before ruling it
> out. I suspect I can come up with something in a day or so. The work
> wouldn't be wasted.

I think it'd be unacceptable to release v11 without support, but I also
think it's quite possible to just add the necessary logic for v11 if we
put some effort into it. ISTM we've resolved worse issues during beta
than this.

Greetings,

Andres Freund

Andres Freund <[hidden email]> writes:
> On 2018-06-19 12:17:56 -0400, Tom Lane wrote:
>> The hard part here is how exactly are we going to represent the default
>> value.  AFAICS, the only thing that pg_dump could readily lay its hands
>> on is the "anyarray" textual representation of attmissingval, which maybe
>> is okay but it means more work for the support function.

> Isn't that just a few lines of code?

Not sure; I've not thought about how to code it.

> And if the default value bugs us,
> we can easily add a support function that dumps the value without the
> anyarray adornment?

The problem here is that that function does not exist in 11beta1.
Since adding the "incoming" function is certainly going to require
initdb, we have to be able to dump from the server as it now stands,
or we'll be cutting existing beta testers adrift.

                        regards, tom lane

Hi,

On 2018-06-19 12:37:52 -0400, Tom Lane wrote:
> Andres Freund <[hidden email]> writes:
> > And if the default value bugs us,
> > we can easily add a support function that dumps the value without the
> > anyarray adornment?
>
> The problem here is that that function does not exist in 11beta1.
> Since adding the "incoming" function is certainly going to require
> initdb, we have to be able to dump from the server as it now stands,
> or we'll be cutting existing beta testers adrift.

It'd probably not be too hard to write a plpgsql replacement for it,
should it come to that. Obviously it'd be nicer to not require users to
create that, but ...

Greetings,

Andres Freund

On Tue, Jun 19, 2018 at 12:37 PM, Tom Lane <[hidden email]> wrote:
> The problem here is that that function does not exist in 11beta1.
> Since adding the "incoming" function is certainly going to require
> initdb, we have to be able to dump from the server as it now stands,
> or we'll be cutting existing beta testers adrift.

That would still be less disruptive than ripping the feature out,
which would be cutting those same users adrift, too, unless I'm
missing something.

I have to admit that I think this feature is scary. I'm not sure that
it was adequately reviewed and tested, and I'm worried this may not be
the only problem it causes. But this particular problem, as Andres
says, doesn't seem like anything we can't fix with acceptable risk.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Andres Freund <[hidden email]> writes:
> On 2018-06-19 12:37:52 -0400, Tom Lane wrote:
>> The problem here is that that function does not exist in 11beta1.
>> Since adding the "incoming" function is certainly going to require
>> initdb, we have to be able to dump from the server as it now stands,
>> or we'll be cutting existing beta testers adrift.

> It'd probably not be too hard to write a plpgsql replacement for it,
> should it come to that. Obviously it'd be nicer to not require users to
> create that, but ...

After some thought, I think it's not that hard to get the support function
to accept the anyarray string form.  I was worried about issues like
whether float8 values would restore exactly, but really that's no worse
than a dump/reload today.  Basically, the support function would just need
to extract the target attribute's type and typmod from the pg_attribute
row, then call array_in().

I wonder though whether there are any interesting corner cases along
this line:

1. Create a column with a fast default.

2. Sometime later, alter the column so that the fast default value
is no longer a legal value.  If the fast default isn't in active use
in the table, the ALTER would go through; but if it does not remove
the attmissingval entry, then ...

3. Subsequently, pg_upgrade fails when the support function tries to
pass the attmissingval entry through the type input function.

The kind of case where this might fail is reducing the allowed
max len (typmod) for a varchar column.  I think ALTER TABLE is
smart enough to not rewrite the table for that, so that there
wouldn't be anything causing the fast default to get removed.

                        regards, tom lane

On Tue, Jun 19, 2018 at 10:12 AM, Robert Haas <[hidden email]> wrote:
> I have to admit that I think this feature is scary. I'm not sure that
> it was adequately reviewed and tested, and I'm worried this may not be
> the only problem it causes. But this particular problem, as Andres
> says, doesn't seem like anything we can't fix with acceptable risk.

I agree with both points.

--
Peter Geoghegan

On 06/19/2018 01:19 PM, Tom Lane wrote:

My experimentation showed this causing a rewrite. I think it only skips
the rewrite if you make the allowed length greater, not smaller.

cheers

andrew

"David G. Johnston" <[hidden email]> writes:
> On Tue, Jun 19, 2018 at 9:37 AM, Tom Lane <[hidden email]> wrote:
>> The problem here is that that function does not exist in 11beta1.
>> Since adding the "incoming" function is certainly going to require
>> initdb, we have to be able to dump from the server as it now stands,
>> or we'll be cutting existing beta testers adrift.

> I was under the impression that we don't promise to support a "v10 -> beta
> -> rc -> final" upgrade path; instead, once final is released people would
> be expected to upgrade "v10 -> v11".

Well, we don't *promise* beta testers that their beta databases will be
usable into production, but ever since pg_upgrade became available we've
tried to make it possible to pg_upgrade to the next beta or production
release.  I do not offhand recall any previous case where we failed to do
so.

                        regards, tom lane

On 06/19/2018 01:19 PM, Tom Lane wrote: This unfortunately crashes and burns if we use DirectFunctionCall3 to
call array_in, because it uses fn_extra. There is the
CallerFInfoFunctionCall stuff, but it only has 1 and 2 arg variants, and
array_in takes 3. In retrospect we should probably have added a 3 arg
form - quite a few input functions take 3 args. Anything else is likely
to be rather uglier.

Attaching the failing patch. I'll attack this again in the morning.

cheers

andrew



--
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-06-19 22:41:26 -0400, Andrew Dunstan wrote:
> This unfortunately crashes and burns if we use DirectFunctionCall3 to call
> array_in, because it uses fn_extra. There is the CallerFInfoFunctionCall
> stuff, but it only has 1 and 2 arg variants, and array_in takes 3. In
> retrospect we should probably have added a 3 arg form - quite a few input
> functions take 3 args. Anything else is likely to be rather uglier.
>
> Attaching the failing patch. I'll attack this again in the morning.

Why don't you just use OidFunctionCall3? Or simply an explicit
fmgr_info(), InitFunctionCallInfoData(), FunctionCallInvoke() combo?

Greetings,

Andres Freund

On 06/19/2018 10:46 PM, Andres Freund wrote:


Thanks for that. I should not code late at night.

Here's a version that works in my testing with Tom's patch making sure
there's a missing value to migrate applied. Thanks to Alvaro for some
useful criticism - any errors are of course my responsibility.


cheers

andrew

--
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

Just a quick scan-through review:

On 2018-06-20 12:51:07 -0400, Andrew Dunstan wrote:
Seems to delete a newline.


Maybe I'm being paranoid, but I feel like the relation should also be
locked here.


> + atttup = SearchSysCacheAttName(table_id,cattname);

Missing space before 'cattname'.


> + if (!HeapTupleIsValid(atttup))
> + elog(ERROR, "cache lookup failed for attribute %s of relation %u",
> + cattname, table_id);
> + attStruct = (Form_pg_attribute) GETSTRUCT(atttup);
> +
> + /* get an array value from the value string */
> +
> + /* find the io info for an arbitrary array type to get array_in Oid */
> + getTypeInputInfo(INT4ARRAYOID, &inputfunc, &inputparam);

Maybe I'm confused, but why INT4ARRAYOID? If you're just looking for the
oid of array_in, why not use F_ARRAY_IN?


> + /* clean up */
> + heap_freetuple(newtup);
> + ReleaseSysCache(atttup);
> + pfree(cattname);
> + pfree(cvalue);
> + pfree(DatumGetPointer(missingval));

Is this worth bothering with (except the ReleaseSysCache)? We'll clean
up via memory context soon, no?

Greetings,

Andres Freund