Horton

Thread

Commits

  1. Move code for the bytea data type from varlena.c to new bytea.c

  1. [PATCH] Split varlena.c into varlena.c and bytea.c

    Aleksander Alekseev <aleksander@timescale.com> — 2025-03-25T11:17:39Z

    Hi,

The proposed patch extracts the code dealing with bytea from varlena.c
into a separate file, as proposed previously [1]. It merely changes
the location of the existing functions. There are no other changes.

[1]: http://postgr.es/m/Zy2UqcZS2XAXBibq%40paquier.xyz

-- 
Best regards,
Aleksander Alekseev

  2. Re: [PATCH] Split varlena.c into varlena.c and bytea.c

    Aleksander Alekseev <aleksander@timescale.com> — 2025-03-31T15:29:54Z

    > The proposed patch extracts the code dealing with bytea from varlena.c
> into a separate file, as proposed previously [1]. It merely changes
> the location of the existing functions. There are no other changes.

Rebased.


--
Best regards,
Aleksander Alekseev

  3. Re: [PATCH] Split varlena.c into varlena.c and bytea.c

    Peter Eisentraut <peter@eisentraut.org> — 2025-06-05T11:47:03Z

    On 31.03.25 17:29, Aleksander Alekseev wrote:
>> The proposed patch extracts the code dealing with bytea from varlena.c
>> into a separate file, as proposed previously [1]. It merely changes
>> the location of the existing functions. There are no other changes.
> 
> Rebased.

I think this is reasonable.  varlena.c is pretty big, and bytea is a 
reasonable subset to take out.  We already have a corresponding header 
file bytea.h, so the naming fits.

I wonder if makeStringAggState() is still useful.  This was factored out 
so that it could be shared between bytea_string_agg_transfn() and 
string_agg_transfn(), but now that these are in separate files, it seems 
to me that it might be easier now to just write the required code inline.

Here are some refinements to the includes:

--- a/src/backend/utils/adt/bytea.c
+++ b/src/backend/utils/adt/bytea.c
@@ -15,13 +15,20 @@
  #include "postgres.h"

  #include "access/detoast.h"
-#include "catalog/pg_collation.h"
+#include "catalog/pg_collation_d.h"
+#include "catalog/pg_type_d.h"
  #include "common/int.h"
-#include "funcapi.h"
+#include "fmgr.h"
  #include "libpq/pqformat.h"
+#include "port/pg_bitutils.h"
  #include "utils/builtins.h"
  #include "utils/bytea.h"
+#include "utils/fmgroids.h"
+#include "utils/fmgrprotos.h"
+#include "utils/memutils.h"
+#include "utils/sortsupport.h"
  #include "utils/varlena.h"
+#include "varatt.h"

Especially funcapi.h was apparently not used.  The other additions are 
required includes that came previously via indirect includes.

  4. Re: [PATCH] Split varlena.c into varlena.c and bytea.c

    Aleksander Alekseev <aleksander@timescale.com> — 2025-06-16T10:33:20Z

    Hi Peter,

> >> The proposed patch extracts the code dealing with bytea from varlena.c
> >> into a separate file, as proposed previously [1]. It merely changes
> >> the location of the existing functions. There are no other changes.
> >
> > Rebased.
>
> I think this is reasonable.  varlena.c is pretty big, and bytea is a
> reasonable subset to take out.  We already have a corresponding header
> file bytea.h, so the naming fits.
>
> I wonder if makeStringAggState() is still useful.  This was factored out
> so that it could be shared between bytea_string_agg_transfn() and
> string_agg_transfn(), but now that these are in separate files, it seems
> to me that it might be easier now to just write the required code inline.

Agree. I assume you meant only bytea.c. Since varlena.c uses
makeStringAggState() in several places I choose to keep it there.

> Here are some refinements to the includes:
>
> --- a/src/backend/utils/adt/bytea.c
> +++ b/src/backend/utils/adt/bytea.c
> @@ -15,13 +15,20 @@
>   #include "postgres.h"
>
>   #include "access/detoast.h"
> -#include "catalog/pg_collation.h"
> +#include "catalog/pg_collation_d.h"
> +#include "catalog/pg_type_d.h"
>   #include "common/int.h"
> -#include "funcapi.h"
> +#include "fmgr.h"
>   #include "libpq/pqformat.h"
> +#include "port/pg_bitutils.h"
>   #include "utils/builtins.h"
>   #include "utils/bytea.h"
> +#include "utils/fmgroids.h"
> +#include "utils/fmgrprotos.h"
> +#include "utils/memutils.h"
> +#include "utils/sortsupport.h"
>   #include "utils/varlena.h"
> +#include "varatt.h"
>
> Especially funcapi.h was apparently not used.  The other additions are
> required includes that came previously via indirect includes.

Thanks, fixed. clangd complained that utils/fmgroids.h is not going to
be used, as it complained about funcapi.h before. So I choose not to
include fmgroids.h.

PFA patch v3.

-- 
Best regards,
Aleksander Alekseev

  5. Re: [PATCH] Split varlena.c into varlena.c and bytea.c

    Michael Paquier <michael@paquier.xyz> — 2025-06-18T05:13:27Z

    On Mon, Jun 16, 2025 at 01:33:20PM +0300, Aleksander Alekseev wrote:
> Thanks, fixed. clangd complained that utils/fmgroids.h is not going to
> be used, as it complained about funcapi.h before. So I choose not to
> include fmgroids.h.

v3 seems sensible here.  Thanks for the updated patch.

/* text_name()
 * Converts a text type to a Name type.
 */

Not related to this patch, sorry for the regression, just noticed
a nit while looking at the diffs of what you have here..  This one, as
well as name_text(), uses a comment block that is inconsistent with
the format we have in the tree.  It's a bit surprising that pgindent
is not picking up that.
--
Michael

  6. Re: [PATCH] Split varlena.c into varlena.c and bytea.c

    Aleksander Alekseev <aleksander@timescale.com> — 2025-06-19T10:39:48Z

    Hi Michael,

> /* text_name()
>  * Converts a text type to a Name type.
>  */
>
> Not related to this patch, sorry for the regression, just noticed
> a nit while looking at the diffs of what you have here..  This one, as
> well as name_text(), uses a comment block that is inconsistent with
> the format we have in the tree.  It's a bit surprising that pgindent
> is not picking up that.

Good observation!

I experimented with pgindent a bit. This is an effect of the -nfc1
flag i.e. format_col1_comments=OFF. Unfortunately just removing it has
undesired side effects so we will have to modify pg_bsd_indent a bit,
apparently somewhere here:

pr_commnet.c:
```
    if (ps.col_1 && !format_col1_comments) {    /* if comment starts in column
                         * 1 it should not be touched */
```

I will start a new thread and propose an appropriate patch.

-- 
Best regards,
Aleksander Alekseev

  7. Re: [PATCH] Split varlena.c into varlena.c and bytea.c

    Michael Paquier <michael@paquier.xyz> — 2025-07-02T00:55:22Z

    On Wed, Jun 18, 2025 at 02:13:27PM +0900, Michael Paquier wrote:
> v3 seems sensible here.  Thanks for the updated patch.

I did a new pass over that.  All the code blocks moved to the new file
are identical.  However, it is really easy to miss the change in
bytea_string_agg_transfn() with the removal of makeStringAggState().
This was mentioned upthread but for somebody who reads only the git
diffs that could be surprising.

I have added a note about that in the commit message, for clarity,
and applied the result.
--
Michael