Feedback-ID: ie0a040ee:Fastmail
Message-ID: <517efb40-dcf8-4ff5-bfa6-c2bbf18ff64e@eisentraut.org>
Date: Wed, 8 Jul 2026 11:05:40 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Wrong query result w/ propgraph single lateral col reference
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>,
 Noah Misch <noah@leadboat.com>
Cc: pgsql-hackers@postgresql.org
References: <20260630173053.51.noahmisch@microsoft.com>
 <CAExHW5sjypF-x1rcutfwxafBxw1mKt4FAQRMFTafMQaZQXAUAg@mail.gmail.com>
 <CAExHW5tdnJDhUUXZ9V8n7aBnW2hqRC0MZgsHnuZj092om3PhQg@mail.gmail.com>
 <CAExHW5sgk_QrxY475uYqrPyW_tMripfM3Z2OoUkkmkj8E_vECg@mail.gmail.com>
Content-Language: en-US
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: 
 <CAExHW5sgk_QrxY475uYqrPyW_tMripfM3Z2OoUkkmkj8E_vECg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/517efb40-dcf8-4ff5-bfa6-c2bbf18ff64e%40eisentraut.org>
Precedence: bulk

On 08.07.26 08:54, Ashutosh Bapat wrote:
>>>> --- a/src/backend/rewrite/rewriteGraphTable.c
>>>> +++ b/src/backend/rewrite/rewriteGraphTable.c
>>>> @@ replace_property_refs(Oid propgraphid, Node *node, const List *mappings)
>>>>          context.mappings = mappings;
>>>>          context.propgraphid = propgraphid;
>>>>
>>>> -       return expression_tree_mutator(node, replace_property_refs_mutator, &context);
>>>> +       return replace_property_refs_mutator(node, &context);
>>>>   }
>>>>
>>>
>>> This matches the pattern in the other expression mutators, and also
>>> fixes the problems. Thanks for the report, that was quite subtle.
>>
>> Fix attached here.
> 
> That's 0003 patch attached here.

committed

>>>> 2. A property whose value is an untyped literal stays type unknown.
>>>>
>>>> CREATE PROPERTY GRAPH gu VERTEX TABLES (v KEY (id) LABEL lu PROPERTIES ('hi' AS p));
>>>> SELECT p FROM GRAPH_TABLE (gu MATCH (n IS lu) COLUMNS (n.p));
>>>> -- ERROR:  failed to find conversion function from unknown to text
>>>>
>>>
>>> I think property's data type should be set to text, not unknown. I
>>> think we should combine the fix for this in
>>> https://www.postgresql.org/message-id/CAHg+QDcyKNWyzDoKMxiZNjv7C-wAxs8y0ZoNkOV137Y+nk3UXg@mail.gmail.com.
>>>
>>
>> Attaching the fix here. But I will also report it on the other thread
>> [2] and discussion can continue there.
>>
> 
> The patch in that thread was committed. Attaching fix for this issue
> as 0002 patch.

committed

>>>> 4. [cosmetic] Duplicate property names found only via catalog unique key
>>>>
>>>> CREATE PROPERTY GRAPH g3 VERTEX TABLES (v KEY (id) LABEL lu PROPERTIES (flag, flag));
>>>> -- ERROR:  duplicate key value violates unique constraint "pg_propgraph_property_name_index"
>>>>
>>>> Opus thinks this is unintentional and cites lack of CommandCounterIncrement()
>>>> between property inserts.
>>>
>>> We get the same error even if we split the duplicate properties across
>>> two commands.
>>> CREATE PROPERTY GRAPH g3 VERTEX TABLES (v KEY (id) LABEL lu PROPERTIES (flag));
>>> alter property graph g3 alter vertex table v alter label lu add
>>> properties(flag);
>>>
>>> I think we need to check for duplicate records in
>>> pg_propgraph_label_property catalog in insert_property_record(). The
>>> function already checks for duplicate records in
>>> pg_propgraph_property. Additionally we might need
>>> CommandCounterIncrement().
>>
> 
> When investigating this issue I found that duplicate labels also have
> the same issue. Fixed both the issues in patch 0001.
> 
> I have added a noop else in insert_label_record() after ereport()
> since it ties the following code block well with the if condition. But
> we can remove it if it's not improving readability.
> 
> There's slight inconsistency in the error messages for duplicate
> labels and properties. In case of labels, we always report "label ...
> already exists". But in case of properties we report two different
> errors. We report "property \"%s\" specified more than once" when
> duplicate properties appear in the same command. But we report
> "property \"%s\" already exists" when duplicate properties appear
> across commands. I think the difference is minor enough that we can
> entirely ignore it and avoid spending more code on it. If we feel
> strongly about being consistent, either we can detect duplicate labels
> in the same command and report "label ... specified more than once" or
> we can make the same error being reported in case of properties in
> both the cases.

I would like this code to be organized differently.  Note that the 
existing insert_*_record functions don't do any error checking; they are 
just there to make catalog modifications.  There are various check_* 
functions that pretty much correspond to syntax rule checks, but these 
are run after the catalog changes, hence the present issue.  Maybe we 
should have a set of "pre-check" functions in addition?