Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Propagate ALTER TABLE ... SET STORAGE to indexes

  1. ALTER TABLE ... SET STORAGE does not propagate to indexes

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-01-06T12:32:53Z

    ALTER TABLE ... SET STORAGE does not propagate to indexes, even though 
indexes created afterwards get the new storage setting.  So depending on 
the order of commands, you can get inconsistent storage settings between 
indexes and tables.  For example:

create table foo1 (a text);
alter table foo1 alter column a set storage external;
create index foo1i on foo1(a);
insert into foo1 values(repeat('a', 10000));
ERROR:  index row requires 10016 bytes, maximum size is 8191

(Storage "external" disables compression.)

but

create table foo1 (a text);
create index foo1i on foo1(a);
alter table foo1 alter column a set storage external;
insert into foo1 values(repeat('a', 10000));
-- no error

Also, this second state cannot be reproduced by pg_dump, so a possible 
effect is that such a database would fail to restore.

Attached is a patch that attempts to fix this by propagating the storage 
change to existing indexes.  This triggers a few regression test 
failures (going from no error to error), which I attempted to fix up, 
but I haven't analyzed what the tests were trying to do, so it might 
need another look.

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

  2. Re: ALTER TABLE ... SET STORAGE does not propagate to indexes

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-02-24T07:28:57Z

    On 2020-01-06 13:32, Peter Eisentraut wrote:
> Attached is a patch that attempts to fix this by propagating the storage
> change to existing indexes.  This triggers a few regression test
> failures (going from no error to error), which I attempted to fix up,
> but I haven't analyzed what the tests were trying to do, so it might
> need another look.

Attached is a more polished patch, with tests.

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

  3. Re: ALTER TABLE ... SET STORAGE does not propagate to indexes

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2020-02-24T11:21:13Z

    Hello Peter,

On Mon, Feb 24, 2020 at 12:59 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
>
> On 2020-01-06 13:32, Peter Eisentraut wrote:
> > Attached is a patch that attempts to fix this by propagating the storage
> > change to existing indexes.  This triggers a few regression test
> > failures (going from no error to error), which I attempted to fix up,
> > but I haven't analyzed what the tests were trying to do, so it might
> > need another look.
>
> Attached is a more polished patch, with tests.

I've reproduced the issue on head. And, the patch seems to solve the
problem. The patch looks good to me. But, I've a small doubt regarding
the changes in test_decoding regression file.

diff --git a/contrib/test_decoding/sql/toast.sql
b/contrib/test_decoding/sql/toast.sql
..
-INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 10000));
-SELECT pg_column_size(toasted_key) > 2^16 FROM toasted_several;
+INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 269));
+SELECT pg_column_size(toasted_key) > 2^11 FROM toasted_several;

This actually tests whether we can decode "old" tuples bigger than the
max heap tuple size correctly which is around 8KB. But, the above
changes will make the tuple size around 3KB. So, it'll not be able to
test that particular scenario.Thoughts?

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  4. Re: ALTER TABLE ... SET STORAGE does not propagate to indexes

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-02-25T07:39:19Z

    On 2020-02-24 12:21, Kuntal Ghosh wrote:
> I've reproduced the issue on head. And, the patch seems to solve the
> problem. The patch looks good to me. But, I've a small doubt regarding
> the changes in test_decoding regression file.
> 
> diff --git a/contrib/test_decoding/sql/toast.sql
> b/contrib/test_decoding/sql/toast.sql
> ..
> -INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 10000));
> -SELECT pg_column_size(toasted_key) > 2^16 FROM toasted_several;
> +INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 269));
> +SELECT pg_column_size(toasted_key) > 2^11 FROM toasted_several;
> 
> This actually tests whether we can decode "old" tuples bigger than the
> max heap tuple size correctly which is around 8KB. But, the above
> changes will make the tuple size around 3KB. So, it'll not be able to
> test that particular scenario.Thoughts?

OK, this is interesting.  The details of this are somewhat unfamiliar to 
me, but it appears that due to TOAST_INDEX_HACK in indextuple.c, an 
index tuple cannot be larger than 8191 bytes when untoasted (but not 
uncompressed).

What the test case above is testing is a situation where the heap tuple 
is stored toasted uncompressed (storage external) but the index tuple is 
not (probably compressed inline).  This is exactly the situation that I 
was contending should not be possible, because it cannot be dumped or 
restored.

An alternative would be that we make this situation fully supported. 
Then we'd probably need at least ALTER INDEX ... ALTER COLUMN ... SET 
STORAGE, and some pg_dump support.

Thoughts?

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

  5. Re: ALTER TABLE ... SET STORAGE does not propagate to indexes

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2020-02-25T13:03:47Z

    On Tue, Feb 25, 2020 at 1:09 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
>
> On 2020-02-24 12:21, Kuntal Ghosh wrote:
> > I've reproduced the issue on head. And, the patch seems to solve the
> > problem. The patch looks good to me. But, I've a small doubt regarding
> > the changes in test_decoding regression file.
> >
> > diff --git a/contrib/test_decoding/sql/toast.sql
> > b/contrib/test_decoding/sql/toast.sql
> > ..
> > -INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 10000));
> > -SELECT pg_column_size(toasted_key) > 2^16 FROM toasted_several;
> > +INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 269));
> > +SELECT pg_column_size(toasted_key) > 2^11 FROM toasted_several;
> >
> > This actually tests whether we can decode "old" tuples bigger than the
> > max heap tuple size correctly which is around 8KB. But, the above
> > changes will make the tuple size around 3KB. So, it'll not be able to
> > test that particular scenario.Thoughts?
>
> OK, this is interesting.  The details of this are somewhat unfamiliar to
> me, but it appears that due to TOAST_INDEX_HACK in indextuple.c, an
> index tuple cannot be larger than 8191 bytes when untoasted (but not
> uncompressed).
>
> What the test case above is testing is a situation where the heap tuple
> is stored toasted uncompressed (storage external) but the index tuple is
> not (probably compressed inline).  This is exactly the situation that I
> was contending should not be possible, because it cannot be dumped or
> restored.
>
Yeah. If we only commit this patch to fix the issue, we're going to
put some restriction for the above situation, i.e., the index for an
external attribute has to be stored as an external (i.e. uncompressed)
value. So, a lot of existing workload might start failing after an
upgrade. I think there should be an option to store the index of an
external attribute as a compressed inline value.

> An alternative would be that we make this situation fully supported.
> Then we'd probably need at least ALTER INDEX ... ALTER COLUMN ... SET
> STORAGE, and some pg_dump support.
>
> Thoughts?
Yes. We need the support for this syntax along with the bug fix patch.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  6. Re: ALTER TABLE ... SET STORAGE does not propagate to indexes

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-30T16:17:20Z

    On 2020-Feb-25, Peter Eisentraut wrote:

> An alternative would be that we make this situation fully supported. Then
> we'd probably need at least ALTER INDEX ... ALTER COLUMN ... SET STORAGE,
> and some pg_dump support.

I think this is a more promising direction.

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

  7. Re: ALTER TABLE ... SET STORAGE does not propagate to indexes

    Adam Brusselback <adambrusselback@gmail.com> — 2020-03-30T16:23:44Z

    > ALTER TABLE ... SET STORAGE does not propagate to indexes, even though
> indexes created afterwards get the new storage setting.  So depending on
> the order of commands, you can get inconsistent storage settings between
> indexes and tables.

I've absolutely noticed this behavior, I just thought it was intentional
for some reason.

Having this behavior change as stated above would be very welcome in my
opinion. It's always something i've had to manually think about in my
migration scripts, so it would be welcome from my view.

-Adam

  8. Re: ALTER TABLE ... SET STORAGE does not propagate to indexes

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-04-09T13:07:40Z

    On 2020-03-30 18:17, Alvaro Herrera wrote:
> On 2020-Feb-25, Peter Eisentraut wrote:
>> An alternative would be that we make this situation fully supported. Then
>> we'd probably need at least ALTER INDEX ... ALTER COLUMN ... SET STORAGE,
>> and some pg_dump support.
> 
> I think this is a more promising direction.

I have started implementing the ALTER INDEX command, which by itself 
isn't very hard, but it requires significant new infrastructure in 
pg_dump, and probably also a bit of work in psql, and that's all a bit 
too much right now.

An alternative for the short term is the attached patch.  It's the same 
as before, but I have hacked up the test_decoding test to achieve the 
effect of ALTER INDEX with direct catalog manipulation.  This preserves 
the spirit of the test case, but allows us to fix everything else about 
this situation.

One thing to remember is that the current situation is broken.  While 
you can set index columns to have different storage than the 
corresponding table columns, pg_dump does not preserve that, because it 
dumps indexes after ALTER TABLE commands.  So at the moment, having 
these two things different isn't really supported.  The proposed patch 
just makes this behave consistently and allows adding an ALTER INDEX 
command later on if desired.

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

  9. Re: ALTER TABLE ... SET STORAGE does not propagate to indexes

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-04-21T23:56:36Z

    I'm surprised that this hasn't applied yet, because:

On 2020-Apr-09, Peter Eisentraut wrote:

> One thing to remember is that the current situation is broken.  While you
> can set index columns to have different storage than the corresponding table
> columns, pg_dump does not preserve that, because it dumps indexes after
> ALTER TABLE commands.  So at the moment, having these two things different
> isn't really supported.

So I have to ask -- are you planning to get this patch pushed and
backpatched?

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

  10. Re: ALTER TABLE ... SET STORAGE does not propagate to indexes

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-04-22T14:26:48Z

    On 2020-04-22 01:56, Alvaro Herrera wrote:
> I'm surprised that this hasn't applied yet, because:
> 
> On 2020-Apr-09, Peter Eisentraut wrote:
> 
>> One thing to remember is that the current situation is broken.  While you
>> can set index columns to have different storage than the corresponding table
>> columns, pg_dump does not preserve that, because it dumps indexes after
>> ALTER TABLE commands.  So at the moment, having these two things different
>> isn't really supported.
> 
> So I have to ask -- are you planning to get this patch pushed and
> backpatched?

I think I should, but I figured I want to give some extra time for 
people to consider the horror that I created in the test_decoding tests.

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

  11. Re: ALTER TABLE ... SET STORAGE does not propagate to indexes

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-05-06T14:37:55Z

    On 2020-04-22 16:26, Peter Eisentraut wrote:
> On 2020-04-22 01:56, Alvaro Herrera wrote:
>> I'm surprised that this hasn't applied yet, because:
>>
>> On 2020-Apr-09, Peter Eisentraut wrote:
>>
>>> One thing to remember is that the current situation is broken.  While you
>>> can set index columns to have different storage than the corresponding table
>>> columns, pg_dump does not preserve that, because it dumps indexes after
>>> ALTER TABLE commands.  So at the moment, having these two things different
>>> isn't really supported.
>>
>> So I have to ask -- are you planning to get this patch pushed and
>> backpatched?
> 
> I think I should, but I figured I want to give some extra time for
> people to consider the horror that I created in the test_decoding tests.

OK then, if there are no last-minute objects, I'll commit this for the 
upcoming minor releases.

This is the patch summary again:

Date: Thu, 9 Apr 2020 14:10:01 +0200
Subject: [PATCH v3] Propagate ALTER TABLE ... SET STORAGE to indexes

When creating a new index, the attstorage setting of the table column
is copied to regular (non-expression) index columns.  But a later
ALTER TABLE ... SET STORAGE is not propagated to indexes, thus
creating an inconsistent and undumpable state.

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

  12. Re: ALTER TABLE ... SET STORAGE does not propagate to indexes

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-05-08T08:17:07Z

    On 2020-05-06 16:37, Peter Eisentraut wrote:
> On 2020-04-22 16:26, Peter Eisentraut wrote:
>> On 2020-04-22 01:56, Alvaro Herrera wrote:
>>> I'm surprised that this hasn't applied yet, because:
>>>
>>> On 2020-Apr-09, Peter Eisentraut wrote:
>>>
>>>> One thing to remember is that the current situation is broken.  While you
>>>> can set index columns to have different storage than the corresponding table
>>>> columns, pg_dump does not preserve that, because it dumps indexes after
>>>> ALTER TABLE commands.  So at the moment, having these two things different
>>>> isn't really supported.
>>>
>>> So I have to ask -- are you planning to get this patch pushed and
>>> backpatched?
>>
>> I think I should, but I figured I want to give some extra time for
>> people to consider the horror that I created in the test_decoding tests.
> 
> OK then, if there are no last-minute objects, I'll commit this for the
> upcoming minor releases.

I have committed this and backpatched to PG12 and PG11.  Before that, 
the catalog manipulation code is factored quite differently and it would 
be more complicated to backpatch and I didn't find that worth it.

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