Re: ZStandard (with dictionaries) compression support for TOAST compression - Mailing list pgsql-hackers

Hi Robert,

Thank you for your feedback on the patch. You’re right that my
proposed design will introduce more dictionary dependencies as
dictionaries grow, I chose this path specifically to avoid changing
existing system behavior and prevent perf regressions in CTAS and
related commands.

After reviewing the email thread you attached on previous response, I
identified a natural choke point for both inserts and updates: the
call to "heap_toast_insert_or_update" inside
heap_prepare_insert/heap_update. In the current master branch, that
function only runs when HeapTupleHasExternal is true; my patch extends
it to HeapTupleHasVarWidth tuples as well. By decompressing every
nested compressed datum at this point—no matter how deeply nested—we
can prevent any leaked datum from propagating into unrelated tables.
This mirrors the existing inlining logic in toast_tuple_init for
external toasted datum, but takes it one step further to fully flatten
datum(decompress datum, not just top level at every level).

On the performance side, my basic benchmarks show almost no regression
for simple INSERT … VALUES workloads. CTAS, however, does regress
noticeably: a CTAS completes in about 4 seconds before this patch, but
with this patch it takes roughly 24 seconds. (For reference, a normal
insert into the source table took about 58 seconds when using zstd
dictionary compression), I suspect the extra cost comes from the added
zstd decompression and PGLZ compression on the destination table.

I’ve attached v13-0008-initial-draft-to-address-datum-leak-problem.patch,
which implements this “flatten_datum” method.

I’d love to know your thoughts on this. Am I on the right track for
solving the problem?

Best regards,
Nikhil Veldanda

On Fri, Apr 18, 2025 at 9:22 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Apr 15, 2025 at 2:13 PM Nikhil Kumar Veldanda
> <veldanda.nikhilkumar17@gmail.com> wrote:
> > Addressing Compressed Datum Leaks problem (via CTAS, INSERT INTO ... SELECT ...)
> >
> > As compressed datums can be copied to other unrelated tables via CTAS,
> > INSERT INTO ... SELECT, or CREATE TABLE ... EXECUTE, I’ve introduced a
> > method inheritZstdDictionaryDependencies. This method is invoked at
> > the end of such statements and ensures that any dictionary
> > dependencies from source tables are copied to the destination table.
> > We determine the set of source tables using the relationOids field in
> > PlannedStmt.
>
> With the disclaimer that I haven't opened the patch or thought
> terribly deeply about this issue, at least not yet, my fairly strong
> suspicion is that this design is not going to work out, for multiple
> reasons. In no particular order:
>
> 1. I don't think users will like it if dependencies on a zstd
> dictionary spread like kudzu across all of their tables. I don't think
> they'd like it even if it were 100% accurate, but presumably this is
> going to add dependencies any time there MIGHT be a real dependency
> rather than only when there actually is one.
>
> 2. Inserting into a table or updating it only takes RowExclusiveLock,
> which is not even self-exclusive. I doubt that it's possible to change
> system catalogs in a concurrency-safe way with such a weak lock. For
> instance, if two sessions tried to do the same thing in concurrent
> transactions, they could both try to add the same dependency at the
> same time.
>
> 3. I'm not sure that CTAS, INSERT INTO...SELECT, and CREATE
> TABLE...EXECUTE are the only ways that datums can creep from one table
> into another. For example, what if I create a plpgsql function that
> gets a value from one table and stores it in a variable, and then use
> that variable to drive an INSERT into another table? I seem to recall
> there are complex cases involving records and range types and arrays,
> too, where the compressed object gets wrapped inside of another
> object; though maybe that wouldn't matter to your implementation if
> INSERT INTO ... SELECT uses a sufficiently aggressive strategy for
> adding dependencies.
>
> When Dilip and I were working on lz4 TOAST compression, my first
> instinct was to not let LZ4-compressed datums leak out of a table by
> forcing them to be decompressed (and then possibly recompressed). We
> spent a long time trying to make that work before giving up. I think
> this is approximately where things started to unravel, and I'd suggest
> you read both this message and some of the discussion before and
> after:
>
> https://www.postgresql.org/message-id/20210316185455.5gp3c5zvvvq66iyj@alap3.anarazel.de
>
> I think we could add plain-old zstd compression without really
> tackling this issue, but if we are going to add dictionaries then I
> think we might need to revisit the idea of preventing things from
> leaking out of tables. What I can't quite remember at the moment is
> how much of the problem was that it was going to be slow to force the
> recompression, and how much of it was that we weren't sure we could
> even find all the places in the code that might need such handling.
>
> I'm now also curious to know whether Andres would agree that it's bad
> if zstd dictionaries are un-droppable. After all, I thought it would
> be bad if there was no way to eliminate a dependency on a compression
> method, and he disagreed. So maybe he would also think undroppable
> dictionaries are fine. But maybe not. It seems even worse to me than
> undroppable compression methods, because you'll probably not have that
> many compression methods ever, but you could have a large number of
> dictionaries eventually.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com