Обсуждение: [PATCH] Remove make_temptable_name_n()

Hi,

The proposed patch removes make_temptable_name_n() in matview.c. This
function is used only once and can be replaced with psprintf().

--
Best regards,
Aleksander Alekseev

On Fri, Oct 10, 2025 at 06:22:46PM +0300, Aleksander Alekseev wrote:
> The proposed patch removes make_temptable_name_n() in matview.c. This
> function is used only once and can be replaced with psprintf().

This dates back to commit cc1965a, and I see some discussion about this
function in the corresponding thread [0].  One thing I don't like about
the patch is that we lose the comment about relying on the name to never be
double-quoted.  IMHO that's worth retaining in some form.

[0] https://postgr.es/m/CAP7Qgm%3Djb3xkzQXfGtX9STx8fzd8EDDQ-oJ8ekcyeOud%2ByLCoA%40mail.gmail.com

-- 
nathan

Hi Nathan,

> This dates back to commit cc1965a, and I see some discussion about this
> function in the corresponding thread [0].  One thing I don't like about
> the patch is that we lose the comment about relying on the name to never be
> double-quoted.  IMHO that's worth retaining in some form.

Fair enough. Here is the corrected patch v2.

-- 
Best regards,
Aleksander Alekseev

On 2025-Oct-15, Aleksander Alekseev wrote:

> @@ -634,7 +611,17 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
>      tempRel = table_open(tempOid, NoLock);
>      tempname = quote_qualified_identifier(get_namespace_name(RelationGetNamespace(tempRel)),
>                                            RelationGetRelationName(tempRel));
> -    diffname = make_temptable_name_n(tempname, 2);
> +    /*
> +     * Create a name for the temporary diff table by appending an underscore
> +     * followed by the given integer to the qualified temporary table name.
> +     * The result is a palloc'd string.
> +     *
> +     * As coded, this would fail to make a valid SQL name if the given name were,
> +     * say, "FOO"."BAR".  Currently, the table name portion of the input will
> +     * never be double-quoted because it's of the form "pg_temp_NNN", cf
> +     * make_new_heap().  But we might have to work harder someday.
> +     */
> +    diffname = psprintf("%s_%d", tempname, 2);

Hmm, but instead of keeping the comment about why this is bogus, why not
just fix it and remove the comment?  You could do something like

nsp = get_namespace_name( .. );
diffname = psprintf("%s_%s_%d", nsp, RelationGetRelationName( .. ), 2);
tempname = quote_qualified_identifier(nsp, RelationGetRelationName( ... ));

and then that should be fairly okay, I think, keeping in mind that both
the names involved are internally-generated short strings -- something
like pg_temp_19.pg_temp_28356_2.

I think it would be better to rewrite this code not to rely on SPI.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Hi Álvaro,

Thanks for your feedback.

> Hmm, but instead of keeping the comment about why this is bogus, why not
> just fix it and remove the comment?  You could do something like
>
> nsp = get_namespace_name( .. );
> diffname = psprintf("%s_%s_%d", nsp, RelationGetRelationName( .. ), 2);
> tempname = quote_qualified_identifier(nsp, RelationGetRelationName( ... ));
>
> and then that should be fairly okay, I think, keeping in mind that both
> the names involved are internally-generated short strings -- something
> like pg_temp_19.pg_temp_28356_2.

Sounds good to me. Here is the updated patch v3.

> I think it would be better to rewrite this code not to rely on SPI.

I will investigate this and start a new thread for better visibility.
This is an invasive change which requires broader discussion.

--
Best regards,
Aleksander Alekseev

Hi,

On Thu, Oct 16, 2025 at 8:11 PM Aleksander Alekseev
<aleksander@tigerdata.com> wrote:
>
> Hi Álvaro,
>
> Thanks for your feedback.
>
> > Hmm, but instead of keeping the comment about why this is bogus, why not
> > just fix it and remove the comment?  You could do something like
> >
> > nsp = get_namespace_name( .. );
> > diffname = psprintf("%s_%s_%d", nsp, RelationGetRelationName( .. ), 2);
> > tempname = quote_qualified_identifier(nsp, RelationGetRelationName( ... ));
> >
> > and then that should be fairly okay, I think, keeping in mind that both
> > the names involved are internally-generated short strings -- something
> > like pg_temp_19.pg_temp_28356_2.
>
> Sounds good to me. Here is the updated patch v3.

Thank you for the patch.

The v1 revision removed make_temptable_name_n and added psprintf,
which reduced the code size. However, the code size in v3 is almost
unchanged, so it's unclear how beneficial this change actually is.

Anyway, I have a minor comment about the patch.

+ char* nsp = get_namespace_name(RelationGetNamespace(tempRel));
+ char* temprelname = RelationGetRelationName(tempRel);
+ char* diffrelname = psprintf("%s_%d", temprelname, 2);

In PostgreSQL code, "char *xxx" seems to be more commonly used than "char* xxx".

--
Best regards,
Shinya Kato
NTT OSS Center

Hi Shinya,

Thanks for your feedback.

> The v1 revision removed make_temptable_name_n and added psprintf,
> which reduced the code size. However, the code size in v3 is almost
> unchanged, so it's unclear how beneficial this change actually is.

Right, the concept has changed a bit, see Álvaro's comment above.

> Anyway, I have a minor comment about the patch.
>
> + char* nsp = get_namespace_name(RelationGetNamespace(tempRel));
> + char* temprelname = RelationGetRelationName(tempRel);
> + char* diffrelname = psprintf("%s_%d", temprelname, 2);
>
> In PostgreSQL code, "char *xxx" seems to be more commonly used than "char* xxx".

My bad, I forgot to run pgindent. Here is the corrected patch.

--
Best regards,
Aleksander Alekseev

On Tue, Oct 21, 2025 at 03:36:46PM +0300, Aleksander Alekseev wrote:
> +    {
> +        char       *nsp = get_namespace_name(RelationGetNamespace(tempRel));
> +        char       *temprelname = RelationGetRelationName(tempRel);
> +        char       *diffrelname = psprintf("%s_%d", temprelname, 2);

I assume the intent of the extra set of curly brackets is to keep the
declarations of these variables close to where they are used.  In this
case, the top of the function is only a few lines up, so IMHO we should
declare them there and save a level of indentation.

> +        pfree(diffrelname);
> +        if (nsp)
> +            pfree(nsp);

Any reason to be so careful about freeing these?  We ordinarily let the
memory context take care of freeing, and refresh_by_match_merge() looks no
different.

-- 
nathan

Hi Nathan,

> > +     {
> > +             char       *nsp = get_namespace_name(RelationGetNamespace(tempRel));
> > +             char       *temprelname = RelationGetRelationName(tempRel);
> > +             char       *diffrelname = psprintf("%s_%d", temprelname, 2);
>
> I assume the intent of the extra set of curly brackets is to keep the
> declarations of these variables close to where they are used.  In this
> case, the top of the function is only a few lines up, so IMHO we should
> declare them there and save a level of indentation.
>
> > +             pfree(diffrelname);
> > +             if (nsp)
> > +                     pfree(nsp);
>
> Any reason to be so careful about freeing these?  We ordinarily let the
> memory context take care of freeing, and refresh_by_match_merge() looks no
> different.

These were just a matter of my personal preferences. I have no strong
opinion on this. Here is the updated patch.

-- 
Best regards,
Aleksander Alekseev

On Wed, Oct 22, 2025 at 04:20:01PM +0300, Aleksander Alekseev wrote:
> Here is the updated patch.

Committed.

-- 
nathan