| From: | Alex Liapychev <coder(dot)sam(at)gmail(dot)com> |
|---|---|
| To: | Jim Jones <jim(dot)jones(at)uni-muenster(dot)de>, tomas(at)vondra(dot)me |
| Cc: | matheusssilv97 <matheusssilv97(at)gmail(dot)com>, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>, "masao(dot)fujii" <masao(dot)fujii(at)gmail(dot)com>, "david(dot)g(dot)johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, tgl <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "huseyin(dot)d3r" <huseyin(dot)d3r(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, chochoforwork(at)gmail(dot)com, 44973863(at)qq(dot)com |
| Subject: | Re: COMMENTS are not being copied in CREATE TABLE LIKE |
| Date: | 2026-09-30 23:14:14 |
| Message-ID: | 36F56BBE-1499-4F82-AC03-05AAAE65D657@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Jim and Tomas, everyone,
I dig the mailing list to find if there was anything related to copying the table comments in the past.
I have found thread starting on 2009-09-07, where “COMMENTS” was added to “INCLUDING …” clause for the first time:
https://www.postgresql.org/message-id/flat/20090907114058.C855.52131E4D%40oss.ntt.co.jp
Part of the discussion in this thread regarding COMMENTS goes around columns, indexes and constraints.
I could not find any mention of table’s own comment.
My personal view is that adding this functionality carries more risks than leaving it out. Although the code is relatively small and appears to work correctly, I would not merge it into the codebase.
An example of how this functionality could be used maliciously:
A workflow creates a new table from several template tables in response to an event.
An adversarial user with sufficient database access adds one comment to each of two template tables. The combined size of these comments exceeds MaxAllocSize, causing the automation to fail unexpectedly.
Kind regards,
Alex Liapychev
> On 27 Sep 2026, at 02:23, Alex Liapychev <coder(dot)sam(at)gmail(dot)com> wrote:
>
> Hi Jim and Tomas, everyone,
>
> Thank you Tomas for stepping in and putting things into perspective.
> Thank you Jim for making the changes so quickly.
>
> We did a recheck of the new version v4 of the patch on September 26, 2026.
> Branch cf/6482 (commit: 47ed4119) rebased over master (commit: f25c50fd).
> Tests on both master alone and with cf/6482 on top completed successfully.
>
> V4 is a complete rewrite.
>
> Author fixed all issues pointed by Tomas and by us (see 3 previous messages).
>
> I will list notable changes briefly:
> 1. Docs: create_table.sgml:695 - replaced “table” for “source relation”, because it could be other types of relations.
> 2. Tests: added INCLUDING ALL test case; test cases for concatenation corner cases; foreign and temporary table as copy target cases; type as a source case.
>
> 3. Code review of a file parse_utilcmd.c:
> 3.1. Includes are now sorted in alphabetical order.
> 3.2. Case when concatenated comment exceeds MaxAllocSize:
>>>> 1. Corner case: concatenation of big comments, whose total size exceeds MaxAllocSize, fails with an error.
>
>>> I'd just reject such cases, with an ERROR that says the comment would be
>>> too long. It's cleaner than just silently start discarding user
>>> information. The number of people hitting this is about 0 anyway. Who
>>> would even have comments of this size?
>
> Implemented on L:1369-1374, error ERRCODE_PROGRAM_LIMIT_EXCEEDED is returned with clear message.
>
> 3.3. Memory allocation and freeing:
>>> FWIW if we really are worried about very long comments, then maybe
>>> expandTableLikeClause should be more careful about freeing comments
>>> during the concatenation? Right now it keeps all the comments and then
>>> also the intermediate strings during concatenation. (I think.)
>
> I tried to trace memory allocations, there are 2 scopes where allocations happen:
>
> 3.3.a. The local scope, where comment of each LIKE relation is read:
> - allocated: in GetComment (L:1348 or 1352);
> - freed: pfree(tblcomment) at L:1379;
> Seems good.
>
> 3.3.b. Scope of transformCreateStmt function:
>
> - defined: Variable cxt (CreateStmtContext) is created on stack (at L:176), inside is new field tablecomment (StringInfo).
> - - It is NULLed before first use at L:257.
>
> - allocated: Field cxt->tablecomment is initialised at L:1361 via makeStringInfo(), which does 2 palloc’s internally.
> - read: cxt.tablecomment->data is stored into cstmt->comment at L:321.
> - freed: NEVER.
>
> destroyStringInfo is not called from parse_utilcmd.c.
> Which seems correct, because we pass data to outside of a function.
> But probably we should call pfree for cxt.tablecomment (StringInfo struct itself) to balance 1st palloc from makeStringInfo.
>
> 3.3.c. (bonus) Scope where de-allocation also might happen:
>
> Comment produced in transformCreateStmt function then reaches code from file comment.c, where it is stored to target relation.
>
> I could not find any pfree for stmt->comment inside the file comment.c, where it is consumed.
> Which should exist to balance 2nd palloc from makeStringInfo.
>
> I don’t know if this palloc'd memory is freed somehow automagically at some later point.
> But to my understanding it may be a memory leak, which spans past this specific patch scope.
>
>
> 4. The question if we should concat comments at all:
>>> That being said, I'm not convinced we actually want to concatenate
>>> comments like this. It feels a bit weird, and it can probably lead to
>>> weird stuff like "duplicate" comments, etc. Do we have any precedent for
>>> this behavior? Are we concatenating comments (or other stuff) anywhere?
>>> I couldn't find such place, but maybe I missed something.
>>
>> Neither am I. I'm just not sure that ignoring the comments when using
>> multiple tables is a better alternative. I can live with it, but so far
>> I didn't find enough arguments to remove it. The alternatives I see are:
>>
>> 1) concatenate with a (\n) separator (current behaviour)
>> 2) concatenate without a separator
>> 3) first one wins
>> 4) last one wins
>> 5) ignore it altogether when multiple comments are detected (my least
>> favourite)
>> 6) your idea? :)
>>
>> WDYT?
>
> I think from perspective of semantics of INCLUDING COMMENTS/ALL it seems right to copy comments of a table.
> But for such complicated scenario, where there are few source relations - not sure.
>
> Maybe "last one wins" would be the easiest to implement and least counter-intuitive for users?
>
> This concatenation code works, but what consequences it may have to support it over years?
>
> Kind regards,
> Alex Liapychev
>
>
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Michael Paquier | 2026-09-30 23:22:22 | Re: Support for 8-byte TOAST values, round two |
| Previous Message | Michael Paquier | 2026-09-30 23:05:20 | Re: injection_points: canceled or terminated waiters leak their wait slots |