Surjective functional indexes

classic Classic list List threaded Threaded
99 messages Options
12345
Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Tom Lane-2
I wrote:
> The bigger picture here, and the reason for my skepticism about having
> any intelligence in the enabling logic, is that there is no scenario
> in which this code can be smarter than the user about what to do.
> We have no insight today, and are unlikely to have any in future, about
> whether a specific index expression is many-to-one or not.

Hmm ... actually, I take that back.  Since we're only interested in this
for expression indexes, we can expect that statistics will be available
for the expression index, at least for tables that have been around
long enough that UPDATE performance is really an exciting topic.
So you could imagine pulling up the stadistinct numbers for the index
column(s) and the underlying column(s), and enabling the optimization
when their ratio is less than $something.  Figuring out how to merge
numbers for multiple columns might be tricky, but it's not going to be
completely fact-free.  (I still think that the cost-estimate logic is
quite bogus, however.)

Another issue in all this is the cost of doing this work over again
after any relcache flush.  Maybe we could move the responsibility
into ANALYZE?

BTW, the existing code appears to be prepared to enable this logic
if *any* index column is an expression, but surely we should do so
only if they are *all* expressions?

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

konstantin knizhnik


On 09.11.2018 2:27, Tom Lane wrote:

> I wrote:
>> The bigger picture here, and the reason for my skepticism about having
>> any intelligence in the enabling logic, is that there is no scenario
>> in which this code can be smarter than the user about what to do.
>> We have no insight today, and are unlikely to have any in future, about
>> whether a specific index expression is many-to-one or not.
> Hmm ... actually, I take that back.  Since we're only interested in this
> for expression indexes, we can expect that statistics will be available
> for the expression index, at least for tables that have been around
> long enough that UPDATE performance is really an exciting topic.
> So you could imagine pulling up the stadistinct numbers for the index
> column(s) and the underlying column(s), and enabling the optimization
> when their ratio is less than $something.  Figuring out how to merge
> numbers for multiple columns might be tricky, but it's not going to be
> completely fact-free.  (I still think that the cost-estimate logic is
> quite bogus, however.)
>
> Another issue in all this is the cost of doing this work over again
> after any relcache flush.  Maybe we could move the responsibility
> into ANALYZE?
>
> BTW, the existing code appears to be prepared to enable this logic
> if *any* index column is an expression, but surely we should do so
> only if they are *all* expressions?
>
> regards, tom lane

 From my point of view "auto" value should be default, otherwise it has
not so much sense.
If somebody decides to switch on this optimization for some particular
index, then it will set it to "on", not "auto".
So I agree with your previous opinion, that if this optimization is
disabled by default, then it is enough to have boolean parameter.

Concerning muticolumn indexes: why we should apply this optimization
only if *all* of index columns are expressions?
Assume very simple example: we have some kind of document storage
represented by the following table:

      create table document(owner integer, name text, last_updated
timestamp, description json);

So there are some static document attributes (name, date,...) and some
dynamic, stored in json field.
Consider that most frequently users will search among their own documents.
So we may create index like this:

      create index by_title on documents(owner,(description->>'title'));

Document description may include many attributes which are updated quite
frequently, like "comments", "keywords",...
But "title" is rarely changed, so this optimization will be very useful
for such index.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

konstantin knizhnik
In reply to this post by Alvaro Herrera-9


On 08.11.2018 22:05, Alvaro Herrera wrote:

> On 2018-Nov-08, Konstantin Knizhnik wrote:
>
>> Before doing any other refactoring of projection indexes I want to attach
>> small bug fix patch which
>> fixes the original problem (SIGSEGV) and also disables recheck_on_update by
>> default.
>> As Laurenz has suggested, I replaced boolean recheck_on_update option with
>> "on","auto,"off" (default).
> I think this causes an ABI break for GenericIndexOpts.  Not sure to what
> extent that is an actual problem (i.e. how many modules were compiled
> with 11.0 that are gonna be reading that struct with later Pg), but I
> think it should be avoided anyhow.
>
Ok, I reverted back my change of reach_on_update option type.
Now if reach_on_update option is not  explicitly specified, then
decision is made based on the expression cost.
Patches becomes very small and fix only error in comparison of index
tuple values.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


projection-11.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Andres Freund
In reply to this post by Tom Lane-2
On 2018-11-07 14:25:54 -0500, Tom Lane wrote:

> Robert Haas <[hidden email]> writes:
> > On Fri, May 11, 2018 at 11:01 AM, Simon Riggs <[hidden email]> wrote:
> >>>> I have no problem if you want to replace this with an even better
> >>>> design in a later release.
>
> >>> Meh. The author / committer should get a patch into the right shape
>
> >> They have done, at length. Claiming otherwise is just trash talk.
>
> > Some people might call it "honest disagreement".
>
> So where we are today is that this patch was lobotomized by commits
> 77366d90f/d06fe6ce2 as a result of this bug report:
>
> https://postgr.es/m/20181106185255.776mstcyehnc63ty@...
>
> We need to move forward, either by undertaking a more extensive
> clean-out, or by finding a path to a version of the code that is
> satisfactory.  I wanted to enumerate my concerns while yesterday's
> events are still fresh in mind.  (Andres or Robert might have more.)
>
> * I do not understand why this feature is on-by-default in the first
> place.  It can only be a win for expression indexes that are many-to-one
> mappings; for indexes that are one-to-one or few-to-one, it's a pretty
> big loss.  I see no reason to assume that most expression indexes fall
> into the first category.  I suggest that the design ought to be to use
> this optimization only for indexes for which the user has explicitly
> enabled recheck_on_update.  That would allow getting rid of the cost check
> in IsProjectionFunctionalIndex, about which we'd otherwise have to have
> an additional fight: I do not like its ad-hoc-ness, nor the modularity
> violation (and potential circularity) involved in having the relcache call
> cost_qual_eval.
>
> * Having heapam.c calling the executor also seems like a
> modularity/layering violation that will bite us in the future.
>
> * The implementation seems incredibly inefficient.  ProjIndexIsUnchanged
> is doing creation/destruction of an EState, index_open/index_close
> (including acquisition of a lock we should already have), BuildIndexInfo,
> and expression compilation, all of which are completely redundant with
> work done elsewhere in the executor.  And it's doing that *over again for
> every tuple*, which totally aside from wasted cycles probably means there
> are large query-lifespan memory leaks in an UPDATE affecting many rows.
> I think a minimum expectation should be that one-time work is done only
> one time; but ideally none of those things would happen at all because
> we could share work with the regular code path.  Perhaps it's too much
> to hope that we could also avoid duplicate computation of the new index
> expression values, but as long as I'm listing complaints ...
>
> * As noted in the bug thread, the implementation of the new reloption
> is broken because (a) it fails to work for some built-in index AMs
> and (b) by design, it can't work for add-on AMs.  I agree with Andres
> that that's not acceptable.
>
> * This seems like bad data structure design:
>
> +   Bitmapset  *rd_projidx;     /* Oids of projection indexes */
>
> That comment is a lie, although IMO it'd be better if it were true;
> a list of target index OIDs would be a far better design here.  The use
> of rd_projidx as a set of indexes into the relation's indexlist is
> inefficient and overcomplicated, plus it creates an unnecessary and not
> very safe (even if it were documented) coupling between rd_indexlist and
> rd_projidx.
>
> * Having ProjIndexIsUnchanged examine relation->rd_projidx directly is
> broken by design anyway, both from a modularity standpoint and because
> its inner loop involves catalog accesses that could result in relcache
> flushes.  I'm somewhat amazed that the regression tests passed on
> CLOBBER_CACHE_ALWAYS buildfarm animals, although possibly this is
> explained by the fact that they're only testing cases with a single
> expression index, so that the bitmap isn't checked again after the cache
> flush happens.  Again, this could be fixed if what was returned was just
> a list of relevant index OIDs.
>
> * This bit of coding is unsafe, for the reason explained in the existing
> comment:
>
>      /*
>       * Now save copies of the bitmaps in the relcache entry.  We intentionally
>       * set rd_indexattr last, because that's the one that signals validity of
>       * the values; if we run out of memory before making that copy, we won't
>       * leave the relcache entry looking like the other ones are valid but
>       * empty.
>       */
>      oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
>      relation->rd_keyattr = bms_copy(uindexattrs);
>      relation->rd_pkattr = bms_copy(pkindexattrs);
>      relation->rd_idattr = bms_copy(idindexattrs);
>      relation->rd_indexattr = bms_copy(indexattrs);
> +    relation->rd_projindexattr = bms_copy(projindexattrs);
> +    relation->rd_projidx = bms_copy(projindexes);
>      MemoryContextSwitchTo(oldcxt);
>
> * There's a lot of other inattention to comments.  For example, I noticed
> that this patch added new responsibilities to RelationGetIndexList without
> updating its header comment to mention them.
>
> * There's a lack of attention to terminology, too.  I do not think that
> "projection index" is an appropriate term at all, nor do I think that
> "recheck_on_update" is a particularly helpful option name, though we
> may be stuck with the latter at this point.
>
> * I also got annoyed by minor sloppiness like not adding the new
> regression test in the same place in serial_schedule and
> parallel_schedule.  The whole thing needed more careful review
> than it got.
>
> In short, it seems likely to me that large parts of this patch need to
> be pulled out, rewritten, and then put back in different places than
> they are today.  I'm not sure if a complete revert is the best next
> step, or if we can make progress without that.

We've not really made progress on this. I continue to think that we
ought to revert this feature, and then work to re-merge it an
architecturally correct way afterwards.  Other opinions?

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On 2018-11-07 14:25:54 -0500, Tom Lane wrote:
>> In short, it seems likely to me that large parts of this patch need to
>> be pulled out, rewritten, and then put back in different places than
>> they are today.  I'm not sure if a complete revert is the best next
>> step, or if we can make progress without that.

> We've not really made progress on this. I continue to think that we
> ought to revert this feature, and then work to re-merge it an
> architecturally correct way afterwards.  Other opinions?

Given the lack of progress, I'd agree with a revert.  It's probably
already going to be a bit painful to undo due to subsequent changes,
so we shouldn't wait too much longer.

Do we want to revert entirely, or leave the "recheck_on_update" option
present but nonfunctional?

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Andres Freund
Hi,

On 2019-01-14 18:03:24 -0500, Tom Lane wrote:

> Andres Freund <[hidden email]> writes:
> > On 2018-11-07 14:25:54 -0500, Tom Lane wrote:
> >> In short, it seems likely to me that large parts of this patch need to
> >> be pulled out, rewritten, and then put back in different places than
> >> they are today.  I'm not sure if a complete revert is the best next
> >> step, or if we can make progress without that.
>
> > We've not really made progress on this. I continue to think that we
> > ought to revert this feature, and then work to re-merge it an
> > architecturally correct way afterwards.  Other opinions?
>
> Given the lack of progress, I'd agree with a revert.  It's probably
> already going to be a bit painful to undo due to subsequent changes,
> so we shouldn't wait too much longer.

Yea, the reason I re-encountered this is cleaning up the pluggable
storage patch. Which certainly would make this revert harder...


> Do we want to revert entirely, or leave the "recheck_on_update" option
> present but nonfunctional?

I think it depends a bit on whether we want to revert in master or
master and 11. If only master, I don't see much point in leaving the
option around. If both, I think we should (need to?) leave it around in
11 only.

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On 2019-01-14 18:03:24 -0500, Tom Lane wrote:
>> Do we want to revert entirely, or leave the "recheck_on_update" option
>> present but nonfunctional?

> I think it depends a bit on whether we want to revert in master or
> master and 11. If only master, I don't see much point in leaving the
> option around. If both, I think we should (need to?) leave it around in
> 11 only.

After a few minutes' more thought, I think that the most attractive
option is to leave v11 alone and do a full revert in HEAD.  In this
way, if anyone's attached "recheck_on_update" options to their indexes,
it'll continue to work^H^H^H^Hdo nothing in v11, though they won't be
able to migrate to v12 till they remove the options.  That way we
aren't bound to the questionable design and naming of that storage
option if/when we try this again.

A revert in v11 would have ABI compatibility issues to think about,
and would also likely be a bunch more work on top of what we'll
have to do for HEAD, so leaving it as-is seems like a good idea.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Stephen Frost
Greetings,

* Tom Lane ([hidden email]) wrote:

> Andres Freund <[hidden email]> writes:
> > On 2019-01-14 18:03:24 -0500, Tom Lane wrote:
> >> Do we want to revert entirely, or leave the "recheck_on_update" option
> >> present but nonfunctional?
>
> > I think it depends a bit on whether we want to revert in master or
> > master and 11. If only master, I don't see much point in leaving the
> > option around. If both, I think we should (need to?) leave it around in
> > 11 only.
>
> After a few minutes' more thought, I think that the most attractive
> option is to leave v11 alone and do a full revert in HEAD.  In this
> way, if anyone's attached "recheck_on_update" options to their indexes,
> it'll continue to work^H^H^H^Hdo nothing in v11, though they won't be
> able to migrate to v12 till they remove the options.  That way we
> aren't bound to the questionable design and naming of that storage
> option if/when we try this again.
So the plan is to add a check into pg_upgrade to complain if it comes
across any cases where recheck_on_update is set during its pre-flight
checks..?

Or are you suggesting that pg_dump in v12+ would throw errors if it
finds that set?  Or that we'll dump it, but fail to allow it into a
v12+ database?  What if v12 sees "recheck_on_update='false'", as a v11
pg_dump might output today?

To be clear, I'm in agreement with reverting this, just trying to think
through what's going to happen and how users will be impacted.

Thanks!

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Andres Freund
On 2019-01-14 18:46:18 -0500, Stephen Frost wrote:

> Greetings,
>
> * Tom Lane ([hidden email]) wrote:
> > Andres Freund <[hidden email]> writes:
> > > On 2019-01-14 18:03:24 -0500, Tom Lane wrote:
> > >> Do we want to revert entirely, or leave the "recheck_on_update" option
> > >> present but nonfunctional?
> >
> > > I think it depends a bit on whether we want to revert in master or
> > > master and 11. If only master, I don't see much point in leaving the
> > > option around. If both, I think we should (need to?) leave it around in
> > > 11 only.
> >
> > After a few minutes' more thought, I think that the most attractive
> > option is to leave v11 alone and do a full revert in HEAD.  In this
> > way, if anyone's attached "recheck_on_update" options to their indexes,
> > it'll continue to work^H^H^H^Hdo nothing in v11, though they won't be
> > able to migrate to v12 till they remove the options.  That way we
> > aren't bound to the questionable design and naming of that storage
> > option if/when we try this again.
>
> So the plan is to add a check into pg_upgrade to complain if it comes
> across any cases where recheck_on_update is set during its pre-flight
> checks..?

I don't think so.

> Or are you suggesting that pg_dump in v12+ would throw errors if it
> finds that set?  Or that we'll dump it, but fail to allow it into a
> v12+ database?  What if v12 sees "recheck_on_update='false'", as a v11
> pg_dump might output today?

It'll just error out on restore (including the internal one by
pg_upgrade). I don't see much point in doing more, this isn't a widely
used option by any stretch.

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Tom Lane-2
In reply to this post by Stephen Frost
Stephen Frost <[hidden email]> writes:
> * Tom Lane ([hidden email]) wrote:
>> After a few minutes' more thought, I think that the most attractive
>> option is to leave v11 alone and do a full revert in HEAD.  In this
>> way, if anyone's attached "recheck_on_update" options to their indexes,
>> it'll continue to work^H^H^H^Hdo nothing in v11, though they won't be
>> able to migrate to v12 till they remove the options.  That way we
>> aren't bound to the questionable design and naming of that storage
>> option if/when we try this again.

> So the plan is to add a check into pg_upgrade to complain if it comes
> across any cases where recheck_on_update is set during its pre-flight
> checks..?

It wasn't my plan particularly.  I think the number of databases with
that option set is probably negligible, not least because it was
on-by-default during its short lifespan.  So there really has never been
a point where someone would have had a reason to turn it on explicitly.

Now if somebody else is excited enough to add such logic to pg_upgrade,
I wouldn't stand in their way.  But I suspect just doing the revert is
already going to be painful enough :-(

> What if v12 sees "recheck_on_update='false'", as a v11
> pg_dump might output today?

It'll complain that that's an unknown option.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Stephen Frost
In reply to this post by Andres Freund
Greetings,

* Andres Freund ([hidden email]) wrote:

> On 2019-01-14 18:46:18 -0500, Stephen Frost wrote:
> > * Tom Lane ([hidden email]) wrote:
> > > Andres Freund <[hidden email]> writes:
> > > > On 2019-01-14 18:03:24 -0500, Tom Lane wrote:
> > > >> Do we want to revert entirely, or leave the "recheck_on_update" option
> > > >> present but nonfunctional?
> > >
> > > > I think it depends a bit on whether we want to revert in master or
> > > > master and 11. If only master, I don't see much point in leaving the
> > > > option around. If both, I think we should (need to?) leave it around in
> > > > 11 only.
> > >
> > > After a few minutes' more thought, I think that the most attractive
> > > option is to leave v11 alone and do a full revert in HEAD.  In this
> > > way, if anyone's attached "recheck_on_update" options to their indexes,
> > > it'll continue to work^H^H^H^Hdo nothing in v11, though they won't be
> > > able to migrate to v12 till they remove the options.  That way we
> > > aren't bound to the questionable design and naming of that storage
> > > option if/when we try this again.
> >
> > So the plan is to add a check into pg_upgrade to complain if it comes
> > across any cases where recheck_on_update is set during its pre-flight
> > checks..?
>
> I don't think so.
I could possibly see us just ignoring the option in pg_dump, but that
goes against what Tom was suggesting, since users wouldn't see an error
if we don't dump the option out..

> > Or are you suggesting that pg_dump in v12+ would throw errors if it
> > finds that set?  Or that we'll dump it, but fail to allow it into a
> > v12+ database?  What if v12 sees "recheck_on_update='false'", as a v11
> > pg_dump might output today?
>
> It'll just error out on restore (including the internal one by
> pg_upgrade). I don't see much point in doing more, this isn't a widely
> used option by any stretch.

This.. doesn't actually make sense.  The pg_upgrade will use v12+
pg_dump.  You're saying that the v12+ pg_dump will dump out the option,
but then the restore will fail to understand it?

Thanks!

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Andres Freund
In reply to this post by Tom Lane-2
Hi,

On 2019-01-14 18:53:02 -0500, Tom Lane wrote:
> But I suspect just doing the revert is already going to be painful
> enough :-(

I assume you're not particularly interested in doing that? I'm more than
happy to leave this to others, but if nobody signals interest I'll give
it a go, because it'll get a lot harder after the pluggable storage work
(and it'd work even less usefully afterwards, given the location in
heapam.c).

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Stephen Frost
In reply to this post by Tom Lane-2
Greetings,

* Tom Lane ([hidden email]) wrote:

> Stephen Frost <[hidden email]> writes:
> > * Tom Lane ([hidden email]) wrote:
> >> After a few minutes' more thought, I think that the most attractive
> >> option is to leave v11 alone and do a full revert in HEAD.  In this
> >> way, if anyone's attached "recheck_on_update" options to their indexes,
> >> it'll continue to work^H^H^H^Hdo nothing in v11, though they won't be
> >> able to migrate to v12 till they remove the options.  That way we
> >> aren't bound to the questionable design and naming of that storage
> >> option if/when we try this again.
>
> > So the plan is to add a check into pg_upgrade to complain if it comes
> > across any cases where recheck_on_update is set during its pre-flight
> > checks..?
>
> It wasn't my plan particularly.  I think the number of databases with
> that option set is probably negligible, not least because it was
> on-by-default during its short lifespan.  So there really has never been
> a point where someone would have had a reason to turn it on explicitly.
>
> Now if somebody else is excited enough to add such logic to pg_upgrade,
> I wouldn't stand in their way.  But I suspect just doing the revert is
> already going to be painful enough :-(
It seems like the thing to do would be to just ignore the option in v12+
pg_dump then, meaning that pg_dump wouldn't output it and
pg_restore/v12+ wouldn't ever see it, and therefore users wouldn't get
an error even if that option was used when they upgrade.

I could live with that, but you seemed to be suggesting that something
else would happen earlier.

> > What if v12 sees "recheck_on_update='false'", as a v11
> > pg_dump might output today?
>
> It'll complain that that's an unknown option.

Right, that's kinda what I figured.  I'm not thrilled with that either,
but hopefully it won't be too big a deal for users.

Thanks!

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Andres Freund
In reply to this post by Stephen Frost
Hi,

On 2019-01-14 18:55:18 -0500, Stephen Frost wrote:

> * Andres Freund ([hidden email]) wrote:
> > > Or are you suggesting that pg_dump in v12+ would throw errors if it
> > > finds that set?  Or that we'll dump it, but fail to allow it into a
> > > v12+ database?  What if v12 sees "recheck_on_update='false'", as a v11
> > > pg_dump might output today?
> >
> > It'll just error out on restore (including the internal one by
> > pg_upgrade). I don't see much point in doing more, this isn't a widely
> > used option by any stretch.
>
> This.. doesn't actually make sense.  The pg_upgrade will use v12+
> pg_dump.  You're saying that the v12+ pg_dump will dump out the option,
> but then the restore will fail to understand it?

Why does that not make sense? With one exception the reloptions code in
pg_dump doesn't have knowledge of individual reloptions. So without
adding any special case code pg_dump will just continue to dump the
option. And the server will just refuse to restore it, because it
doesn't know it anymore.

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Tom Lane-2
In reply to this post by Andres Freund
Andres Freund <[hidden email]> writes:
> On 2019-01-14 18:53:02 -0500, Tom Lane wrote:
>> But I suspect just doing the revert is already going to be painful
>> enough :-(

> I assume you're not particularly interested in doing that?

No, I'm willing to do it, and will do so tomorrow if there haven't
been objections.

What I'm not willing to do is write hacks for pg_upgrade or pg_dump
to mask cases where the option has been set on a v11 index.  I judge
that it's not worth the trouble.  If someone else disagrees, they
can do that work.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Stephen Frost
In reply to this post by Andres Freund
Greetings,

* Andres Freund ([hidden email]) wrote:

> On 2019-01-14 18:55:18 -0500, Stephen Frost wrote:
> > * Andres Freund ([hidden email]) wrote:
> > > > Or are you suggesting that pg_dump in v12+ would throw errors if it
> > > > finds that set?  Or that we'll dump it, but fail to allow it into a
> > > > v12+ database?  What if v12 sees "recheck_on_update='false'", as a v11
> > > > pg_dump might output today?
> > >
> > > It'll just error out on restore (including the internal one by
> > > pg_upgrade). I don't see much point in doing more, this isn't a widely
> > > used option by any stretch.
> >
> > This.. doesn't actually make sense.  The pg_upgrade will use v12+
> > pg_dump.  You're saying that the v12+ pg_dump will dump out the option,
> > but then the restore will fail to understand it?
>
> Why does that not make sense? With one exception the reloptions code in
> pg_dump doesn't have knowledge of individual reloptions. So without
> adding any special case code pg_dump will just continue to dump the
> option. And the server will just refuse to restore it, because it
> doesn't know it anymore.
Ugh, yeah, I was catching up to realize that we'd have to special-case
add this into pg_dump to get it avoided; most things in pg_dump don't
work that way.

As that's the case, then I guess I'm thinking we really should make
pg_upgrade complain if it finds it during the check phase.  I really
don't like having a case like this where the pg_upgrade will fail from
something that we could have detected during the pre-flight check,
that's what it's for, after all.

Thanks,

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Stephen Frost
In reply to this post by Tom Lane-2
Greetings,

* Tom Lane ([hidden email]) wrote:

> Andres Freund <[hidden email]> writes:
> > On 2019-01-14 18:53:02 -0500, Tom Lane wrote:
> >> But I suspect just doing the revert is already going to be painful
> >> enough :-(
>
> > I assume you're not particularly interested in doing that?
>
> No, I'm willing to do it, and will do so tomorrow if there haven't
> been objections.
>
> What I'm not willing to do is write hacks for pg_upgrade or pg_dump
> to mask cases where the option has been set on a v11 index.  I judge
> that it's not worth the trouble.  If someone else disagrees, they
> can do that work.
I'd love for there to be a better option beyond "just let people run the
pg_upgrade and have it fail half-way through"...  Particularly after
someone's run the pg_upgrade check against the database...

If there isn't, then I'll write the code to add the check to pg_upgrade.

I'll also offer to add other such checks, if there's similar cases that
people are aware of.

Thanks,

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Andres Freund
In reply to this post by Stephen Frost
Hi,

On 2019-01-14 19:04:07 -0500, Stephen Frost wrote:
> As that's the case, then I guess I'm thinking we really should make
> pg_upgrade complain if it finds it during the check phase.  I really
> don't like having a case like this where the pg_upgrade will fail from
> something that we could have detected during the pre-flight check,
> that's what it's for, after all.

I suggest you write a separate patch for that in that case.

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: Surjective functional indexes

Tom Lane-2
In reply to this post by Stephen Frost
Stephen Frost <[hidden email]> writes:
> * Tom Lane ([hidden email]) wrote:
>> What I'm not willing to do is write hacks for pg_upgrade or pg_dump
>> to mask cases where the option has been set on a v11 index.  I judge
>> that it's not worth the trouble.  If someone else disagrees, they
>> can do that work.

> I'd love for there to be a better option beyond "just let people run the
> pg_upgrade and have it fail half-way through"...  Particularly after
> someone's run the pg_upgrade check against the database...

> If there isn't, then I'll write the code to add the check to pg_upgrade.

Go for it.

                        regards, tom lane

12345