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

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

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.

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

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

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

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