On Thu, Jun 10, 2021 at 11:09:52AM +0900, Michael Paquier wrote:
> On Tue, Jun 08, 2021 at 11:32:21PM -0400, Tom Lane wrote:
>> In the meantime I'm +1 for dropping this logic from VACUUM FULL.
>> I don't even want to spend enough more time on it to confirm the
>> different overhead measurements
On Tue, Jun 08, 2021 at 11:32:21PM -0400, Tom Lane wrote:
> I can imagine sometime in the future where we need to get rid of all
> instances of pglz so we can reassign that compression code to something
> else. But would we insist on a mass VACUUM FULL to make that happen?
> Doubt it. You'd want
Michael Paquier writes:
> On Tue, Jun 08, 2021 at 10:39:24AM -0400, Alvaro Herrera wrote:
>> Maybe at this point reverting is the only solution.
> That's a safe bet at this point. It would be good to conclude this
> one by beta2 IMO.
I still think it's a really dubious argument that anybody
On Tue, Jun 08, 2021 at 10:39:24AM -0400, Alvaro Herrera wrote:
> My unverified guess is that this code causes too many pipeline stalls
> while executing the big per-column loop. Maybe it would be better to
> scan the attribute array twice: one to collect all data from
> Form_pg_attribute for
On 2021-Jun-06, Michael Paquier wrote:
> On Fri, Jun 04, 2021 at 07:21:11PM -0400, Alvaro Herrera wrote:
> The test has been done with four configurations, and here are the
> results:
> 1) HEAD: 9659ms
> 2) REL_13_STABLE: 8310ms.
> 3) Alvaro's patch, as of
>
Em sáb., 5 de jun. de 2021 às 11:16, Ranier Vilela
escreveu:
> > Alvaro Herrera writes:
>
> > > Now, while this patch does seem to work correctly, it raises a number
> of
> > > weird cpluspluscheck warnings, which I think are attributable to the
> > > new macro definitions. I didn't look into
On Fri, Jun 04, 2021 at 07:21:11PM -0400, Alvaro Herrera wrote:
> On 2021-Jun-04, Tom Lane wrote:
>
> > Alvaro Herrera writes:
>
> > > Now, while this patch does seem to work correctly, it raises a number of
> > > weird cpluspluscheck warnings, which I think are attributable to the
> > > new
> Alvaro Herrera writes:
> > Now, while this patch does seem to work correctly, it raises a number of
> > weird cpluspluscheck warnings, which I think are attributable to the
> > new macro definitions. I didn't look into it closely, but I suppose it
> > should be fixable given sufficient effort:
On 2021-Jun-04, Tom Lane wrote:
> Alvaro Herrera writes:
> > Now, while this patch does seem to work correctly, it raises a number of
> > weird cpluspluscheck warnings, which I think are attributable to the
> > new macro definitions. I didn't look into it closely, but I suppose it
> > should
Alvaro Herrera writes:
> It seems to me that most of the overhead is the function call for
> toast_get_compression_id(), so we should get rid of that.
Nice result. I'm willing to live with 1.5% slowdown ... IME that's
usually below the noise threshold anyway.
> Now, while this patch does seem
So I tried running vacuum full in pgbench of your 10-column table,
max_wal_size=32GB. I didn't move pgdata to an in-memory pgdata, but
this is on NVMe so pretty fast anyway.
pgbench -c1 -t30 -n -f vacuumfull.sql.
Current master:
latency average = 2885.550 ms
latency stddev = 1771.170 ms
tps =
On Fri, Jun 04, 2021 at 08:54:48AM +0900, Michael Paquier wrote:
> I have done no recompression here, so I was just stressing the extra
> test for the recompression. Sorry for the confusion.
I am not sure yet which way we are going, but cleaning up this code
involves a couple of things:
- Clean
On Thu, Jun 03, 2021 at 12:04:48PM -0400, Alvaro Herrera wrote:
> If the check for recompression is this expensive, then yeah I agree that
> we should take it out. If recompression is actually occurring, then I
> don't think this is a good test :-)
I have done no recompression here, so I was
On 2021-Jun-02, Michael Paquier wrote:
> After 12 runs of VACUUM FULL on my laptop, I have removed the two
> highest and the two lowest to remove some noise, and did an average of
> the rest:
> - HEAD, 100 text columns: 5720ms
> - REL_13_STABLE, 100 text columns: 4308ms
> - HEAD, 200 text
On Thu, May 27, 2021 at 03:52:06PM -0700, Peter Geoghegan wrote:
> On Thu, May 27, 2021 at 1:29 PM Tom Lane wrote:
>> Yeah. My belief here is that users might bother to change
>> default_toast_compression, or that we might do it for them in a few
>> years, but the gains from doing so are going
On Thu, May 27, 2021 at 04:17:58PM -0400, Tom Lane wrote:
> BTW, perhaps I should clarify my goal here: it's to cut off expending
> further effort on this feature during v14.
No disagreement here.
> If we can decide that the
> existing performance situation is acceptable, I'm content with that
>
On Thu, May 27, 2021 at 1:29 PM Tom Lane wrote:
> Yeah. My belief here is that users might bother to change
> default_toast_compression, or that we might do it for them in a few
> years, but the gains from doing so are going to be only incremental.
> That being the case, most DBAs will be
On Tue, May 25, 2021 at 08:33:47PM -0500, Justin Pryzby wrote:
> On Sun, May 23, 2021 at 12:25:10PM -0400, Tom Lane wrote:
> > However, the more I looked at that code the less I liked it.
> > I think the way that compression selection is handled for indexes,
> > ie consult
Peter Geoghegan writes:
> On Thu, May 27, 2021 at 7:25 AM Robert Haas wrote:
>> To say that nobody cares about that is to deem the
>> feature useless. Maybe that's what Tom thinks, but it's not what I
>> think.
> I don't think that follows at all.
Yeah. My belief here is that users might
Alvaro Herrera writes:
> Now about the former. If we do think that recompressing causes an
> unacceptable 10% slowdown for every single VACUUM FULLs, then yeah we
> should discuss changing that behavior -- maybe remove promises of
> recompression and wait for pg15 to add "VACUUM (RECOMPRESS)" or
Alvaro Herrera writes:
> Sorry, I'm unclear on exactly what are you proposing. Are you proposing
> to rip out the fact that VACUUM FULL promises to recompress everything,
> or are you proposing to rip out the whole attcompression feature?
Just the former.
regards, tom
On Thu, May 27, 2021 at 7:25 AM Robert Haas wrote:
> TBH, I'm more concerned about the other direction. Surely someone who
> upgrades from an existing release to v14 and sets their compression
> method to lz4 is going to want a way of actually converting their data
> to using lz4.
Your argument
On 2021-May-27, Tom Lane wrote:
> I say it's time to stop the bleeding and rip it out. When and if
> there are actual field requests to have a way to do this, we can
> discuss what's the best way to respond to those requests. Hacking
> VACUUM probably isn't the best answer, anyway. But right
On Thu, May 27, 2021 at 10:39 AM Tom Lane wrote:
> What I'm hearing is a whole lot of hypothesizing and zero evidence of
> actual field requirements. On the other side of the coin, we've already
> wasted significant person-hours on fixing this feature's memory leakage,
> and now people are
Robert Haas writes:
> On Thu, May 27, 2021 at 10:18 AM Dilip Kumar wrote:
>>> [ shrug... ] I think the history of the SET STORAGE option teaches us
>>> that there is no such requirement, and you're inventing a scenario that
>>> doesn't exist in the real world.
>> But can we compare SET STORAGE
On Thu, May 27, 2021 at 10:18 AM Dilip Kumar wrote:
> > [ shrug... ] I think the history of the SET STORAGE option teaches us
> > that there is no such requirement, and you're inventing a scenario that
> > doesn't exist in the real world.
>
> But can we compare SET STORAGE with SET compression?
On Thu, May 27, 2021 at 7:04 PM Tom Lane wrote:
>
> Robert Haas writes:
> > On Thu, May 27, 2021 at 12:11 AM Tom Lane wrote:
> >> AFAIR, there are zero promises about how effective, or when effective,
> >> changes in SET STORAGE will be. And the number of complaints about
> >> that has also
Robert Haas writes:
> On Thu, May 27, 2021 at 12:11 AM Tom Lane wrote:
>> AFAIR, there are zero promises about how effective, or when effective,
>> changes in SET STORAGE will be. And the number of complaints about
>> that has also been zero. So I'm not sure why we need to do more for
>> SET
On Thu, May 27, 2021 at 12:11 AM Tom Lane wrote:
> AFAIR, there are zero promises about how effective, or when effective,
> changes in SET STORAGE will be. And the number of complaints about
> that has also been zero. So I'm not sure why we need to do more for
> SET COMPRESSION. Especially
Michael Paquier writes:
> On Wed, May 26, 2021 at 11:34:53PM -0400, Tom Lane wrote:
>> So maybe we should just dump the promise that VACUUM FULL will recompress
>> everything? I'd be in favor of that actually, because it seems 100%
>> outside the charter of either VACUUM FULL or CLUSTER.
> Hmm.
On Wed, May 26, 2021 at 11:34:53PM -0400, Tom Lane wrote:
> So maybe we should just dump the promise that VACUUM FULL will recompress
> everything? I'd be in favor of that actually, because it seems 100%
> outside the charter of either VACUUM FULL or CLUSTER.
Hmm. You are right that by default
Andres Freund writes:
> Yea, I tested that - it does help in the integer case. But the bigger
> contributors are the loop over the attributes, and especially the access
> to the datum's compression method. Particularly the latter seems hard to
> avoid.
So maybe we should just dump the promise
Hi,
On 2021-05-26 22:43:42 -0400, Tom Lane wrote:
> The memsets seem to be easy to get rid of. memset the array
> to zeroes *once* before entering the per-tuple loop. Then,
> in the loop that looks for stuff to pfree, reset any entries
> that are found to be set, thereby returning the array to
Andres Freund writes:
> That's not *too* bad, but also not nothing
The memsets seem to be easy to get rid of. memset the array
to zeroes *once* before entering the per-tuple loop. Then,
in the loop that looks for stuff to pfree, reset any entries
that are found to be set, thereby returning
Hi,
On 2021-05-27 11:07:53 +0900, Michael Paquier wrote:
> This depends on the number of attributes, but I do see an extra 0.5%
> __memmove_avx_unaligned_erms in reform_and_rewrite_tuple() for a
> normal VACUUM FULL with a 1-int-column relation on a perf profile,
> with rewrite_heap_tuple eating
Hi,
On 2021-05-26 18:54:15 -0700, Andres Freund wrote:
> On 2021-05-26 20:35:46 -0400, Tom Lane wrote:
> > Andres Freund writes:
> > > The efficiency bit is probably going to be swamped by the addition of
> > > the compression handling, given the amount of additional work we're now
> > > doing
On Wed, May 26, 2021 at 06:54:15PM -0700, Andres Freund wrote:
> Oh, it'll definitely be more expensive in that case - but that seems
> fair game. What I was wondering about was whether VACUUM FULL would be
> measurably slower, because we'll now call toast_get_compression_id() on
> each varlena
Hi,
On 2021-05-26 20:35:46 -0400, Tom Lane wrote:
> Andres Freund writes:
> > The efficiency bit is probably going to be swamped by the addition of
> > the compression handling, given the amount of additional work we're now
> > doing in in reform_and_rewrite_tuple().
>
> Only if the user has
On Wed, May 26, 2021 at 08:35:46PM -0400, Tom Lane wrote:
> Andres Freund writes:
> > The efficiency bit is probably going to be swamped by the addition of
> > the compression handling, given the amount of additional work we're now
> > doing in in reform_and_rewrite_tuple().
>
> Only if the user
Andres Freund writes:
> The efficiency bit is probably going to be swamped by the addition of
> the compression handling, given the amount of additional work we're now
> doing in in reform_and_rewrite_tuple().
Only if the user has explicitly requested a change of compression, no?
Hi,
On 2021-05-25 14:46:27 +0900, Michael Paquier wrote:
> That would work. Your suggestion, as I understood it first, makes the
> code simpler by not using tup_values at all as the set of values[] is
> filled when the values and nulls are extracted. So I have gone with
> this simplification,
Michael Paquier writes:
> Yeah, having an extra test for partitioned tables would be a good
> idea.
We do have some coverage already via the pg_upgrade test.
> Could it be possible to have some tests for COMPRESSION DEFAULT? It
> seems to me that this should be documented as a supported
On Wed, May 26, 2021 at 07:44:03PM -0400, Alvaro Herrera wrote:
> On 2021-May-26, Tom Lane wrote:
>> Personally I won't touch 002_pg_dump.pl with a 10-foot pole, but if
>> somebody else wants to, have at it.
>
> Nod.
Yeah, having an extra test for partitioned tables would be a good
idea.
>> Hm,
On 2021-May-26, Tom Lane wrote:
> Alvaro Herrera writes:
> > It would be good to have a test case in src/bin/pg_dump/t/002_pg_dump.pl
> > for the case
>
> Personally I won't touch 002_pg_dump.pl with a 10-foot pole, but if
> somebody else wants to, have at it.
Nod.
> > ... and I find it odd
Alvaro Herrera writes:
> Looks good to me.
> I tested the behavior with partitioned tables and it seems OK.
Thanks for reviewing/testing!
> It would be good to have a test case in src/bin/pg_dump/t/002_pg_dump.pl
> for the case
Personally I won't touch 002_pg_dump.pl with a 10-foot pole, but
On 2021-May-26, Tom Lane wrote:
> I wrote:
> > I think this is about ready to commit now (though I didn't yet nuke
> > GetDefaultToastCompression).
>
> Here's a bundled-up final version, in case anybody would prefer
> to review it that way.
Looks good to me.
I tested the behavior with
I wrote:
> I think this is about ready to commit now (though I didn't yet nuke
> GetDefaultToastCompression).
Here's a bundled-up final version, in case anybody would prefer
to review it that way.
regards, tom lane
diff --git a/doc/src/sgml/catalogs.sgml
On Wed, May 26, 2021 at 11:13:46AM -0400, Tom Lane wrote:
> Michael Paquier writes:
> > Ah, the parallel with reltablespace and default_tablespace at database
> > level is a very good point. It is true that currently the code would
> > assign attcompression to a non-zero value once the relation
Robert Haas writes:
> On Wed, May 26, 2021 at 11:13 AM Tom Lane wrote:
>> * As things stand here, once you've applied ALTER ... SET COMPRESSION
>> to select a specific method, there is no way to undo that and go
>> back to the use-the-default setting. All you can do is change to
>> explicitly
On Wed, May 26, 2021 at 11:13 AM Tom Lane wrote:
> * As things stand here, once you've applied ALTER ... SET COMPRESSION
> to select a specific method, there is no way to undo that and go
> back to the use-the-default setting. All you can do is change to
> explicitly select the other method.
Michael Paquier writes:
> Ah, the parallel with reltablespace and default_tablespace at database
> level is a very good point. It is true that currently the code would
> assign attcompression to a non-zero value once the relation is defined
> depending on default_toast_compression set for the
On Tue, May 25, 2021 at 08:33:47PM -0500, Justin Pryzby wrote:
> It reminds me of reltablespace, which is stored as 0 to mean the database's
> default tablespace.
>
> Also, values are currently retoasted during vacuum full if their column's
> current compression method doesn't match the value's
On Sun, May 23, 2021 at 12:25:10PM -0400, Tom Lane wrote:
> However, the more I looked at that code the less I liked it.
> I think the way that compression selection is handled for indexes,
> ie consult default_toast_compression on-the-fly, is *far* saner
> than what is currently implemented for
On Tue, 25 May 2021 at 11:16 AM, Michael Paquier
wrote:
> On Mon, May 24, 2021 at 02:46:11PM +0530, Dilip Kumar wrote:
> > Like this.
> > if (TupleDescAttr(newTupDesc, i)->attisdropped)
> > isnull[i] = true;
> > else
> > tup_values[i] = values[i];
>
> That would work. Your
On Mon, May 24, 2021 at 02:46:11PM +0530, Dilip Kumar wrote:
> Like this.
> if (TupleDescAttr(newTupDesc, i)->attisdropped)
> isnull[i] = true;
> else
> tup_values[i] = values[i];
That would work. Your suggestion, as I understood it first, makes the
code simpler by not using
On Mon, May 24, 2021 at 2:23 PM Michael Paquier wrote:
>
> On Mon, May 24, 2021 at 11:32:22AM +0530, Dilip Kumar wrote:
> > I think you don't need to initialize tup_values[i] with the
> > values[i];, other than that looks fine to me.
>
> You mean because heap_deform_tuple() does this job, right?
On Mon, May 24, 2021 at 11:32:22AM +0530, Dilip Kumar wrote:
> I think you don't need to initialize tup_values[i] with the
> values[i];, other than that looks fine to me.
You mean because heap_deform_tuple() does this job, right? Sure.
--
Michael
signature.asc
Description: PGP signature
On Mon, May 24, 2021 at 9:39 AM Michael Paquier wrote:
>
> On Fri, May 21, 2021 at 02:19:29PM -0700, Andres Freund wrote:
> > During VACUUM FULL reform_and_rewrite_tuple() detoasts the old value if
> > it was compressed with a different method, while in
> > TopTransactionContext. There's nothing
On Fri, May 21, 2021 at 8:31 PM Tom Lane wrote:
>
>
> if (rel->rd_rel->relkind == RELKIND_RELATION ||
> rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
>
> This seems fairly nuts; in particular, why are matviews excluded?
The matviews are excluded only in "ATExecAddColumn()"
Michael Paquier writes:
> On Sun, May 23, 2021 at 12:25:10PM -0400, Tom Lane wrote:
>> While I've not actually tested this, it seems to me that we could
>> just drop these relkind tests altogether. It won't hurt anything
>> to set up attcompression in relation descriptors where it'll never
>> be
On Sun, May 23, 2021 at 12:25:10PM -0400, Tom Lane wrote:
> While I've not actually tested this, it seems to me that we could
> just drop these relkind tests altogether. It won't hurt anything
> to set up attcompression in relation descriptors where it'll never
> be consulted.
Wouldn't it be
On Fri, May 21, 2021 at 02:19:29PM -0700, Andres Freund wrote:
> During VACUUM FULL reform_and_rewrite_tuple() detoasts the old value if
> it was compressed with a different method, while in
> TopTransactionContext. There's nothing freeing that until
> TopTransactionContext ends - obviously not
I wrote:
> I think we need to do more than that. It's certainly not okay to
> leave catalogs.sgml out of sync with reality. And maybe I'm just
> an overly anal-retentive sort, but I think that code that manipulates
> tuples ought to match the declared field order if there's not some
> specific
Hi,
On 2021-05-21 11:01:03 -0400, Tom Lane wrote:
> It was a good thing I went through this code, too, because I noticed
> one serious bug (attcompression not checked in equalTupleDescs) and
> another thing that looks like a bug:
Grepping for attcompression while trying to understand the issue
Hi,
On 2021-05-21 11:01:03 -0400, Tom Lane wrote:
> It was a good thing I went through this code, too, because I noticed
> one serious bug (attcompression not checked in equalTupleDescs) and
> another thing that looks like a bug: there are two places that set
> up attcompression depending on
>
>
Michael Paquier writes:
> This is still an open item. FWIW, I can get behind the reordering
> proposed by Tom for the consistency gained with pg_type, leading to
> the attached to reduce the size of FormData_pg_attribute from 116b to
> 112b.
I think we need to do more than that. It's certainly
On Fri, May 21, 2021 at 12:02 PM Michael Paquier wrote:
>
> On Tue, May 18, 2021 at 10:24:36AM +0900, Michael Paquier wrote:
> > If you switch attcompression, I'd say to go for the others while on
> > it. It would not be the first time in history there is a catalog
> > version bump between
On Tue, May 18, 2021 at 10:24:36AM +0900, Michael Paquier wrote:
> If you switch attcompression, I'd say to go for the others while on
> it. It would not be the first time in history there is a catalog
> version bump between betas.
This is still an open item. FWIW, I can get behind the
On Mon, May 17, 2021 at 02:28:57PM -0700, Andres Freund wrote:
> On 2021-05-17 17:06:32 -0400, Tom Lane wrote:
>> Putting it just after attalign seems like a reasonably sane choice
>> from the standpoint of grouping things affecting physical storage;
>> and as you say, that wins from the
Hi,
On 2021-05-17 17:06:32 -0400, Tom Lane wrote:
> Putting it just after attalign seems like a reasonably sane choice
> from the standpoint of grouping things affecting physical storage;
> and as you say, that wins from the standpoint of using up alignment
> padding rather than adding more.
Andres Freund writes:
> If we moved attcompression to all the other bool/char fields, we'd avoid
> that size increase, as there's an existing 2 byte hole.
+1. Looks to me like its existing placement was according to the good
old "add new things at the end" anti-pattern. It certainly isn't
On Mon, May 17, 2021 at 01:48:03PM -0700, Andres Freund wrote:
> pg_attribute is one of the biggest table in a new cluster, and often the
> biggest table in production clusters. Its size is also quite relevant in
> memory, due to all the TupleDescs we allocate.
>
> I just noticed that the new
Hi,
pg_attribute is one of the biggest table in a new cluster, and often the
biggest table in production clusters. Its size is also quite relevant in
memory, due to all the TupleDescs we allocate.
I just noticed that the new attcompression increased the size not just
by 1 byte, but by 4, due to
73 matches
Mail list logo
