Re: COMMENTS are not being copied in CREATE TABLE LIKE - Mailing list pgsql-hackers

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
targetcases; 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