Re: Vacuumlo improvements

Hi,

On Tue, Jun 2, 2026 at 2:05 PM Sami Imseih <samimseih(at)gmail(dot)com> wrote:
>

Thanks for the patches. I started reading the code and docs around the
LOs for my learning before this patch review.

> > > But with all this done, I am not sure how much this moves the needle. It may
> > > somewhat, but it's hard to tell how much. I know I have seen users store
> > > LO references in text or other types, so I think we still need the
> > > documentation enhancement to call out the "data loss" potential.
> > >
> > > I also think it will be good for the LO documentation [1] to nudge the users
> > > to think about using the LO extension, as is done with the vacuumlo [2]
> > > documentation.
> >
> > Yeah, I think we'll have to do some combination of 1) improving vacuumlo,
> > 2) improving the documentation to warn users about things vacuumlo doesn't
> > catch, and 3) improving the documentation to nudge users toward the lo
> > extension..

I agree to do something here. Improving the docs seems the first thing
to do. Also improving vacuumlo to detect the *possible* ways users can
store LO OIDs.

> See the attached patches.
>
> 0001 - vacuumlo now can recursively search for OID or LO types inside
> more complex
> data types. It does so by having the query return the search
> expression depending on the type
> to the delete statement.

1/ While I'm, in general, okay to add more tests, I think the added
ones look like *some* patterns users can follow to store the LO refs.
Can we just simplify the 0001 with the one reported case of domains
and shorten it and split the others to 0003?

2/ Why TAP test and start a new database instance? Why not add a sql
test with the one reported domain case and be done with it?

3/ Looks like client-side encoding is not moved to the server-side
encoding? Does this have any impact on the different eoncodings or
such? Maybe I'm over cautious here. Can we avoid this in this patch
and if it's useful on its own, split it to a separate patch?

- schema = PQescapeIdentifier(conn, schema, strlen(schema));
- table = PQescapeIdentifier(conn, table, strlen(table));
- field = PQescapeIdentifier(conn, field, strlen(field));

4/ We now seem to be constructing the expression in the CTE, can this
ever be more than 4K? Or should we use PQExpBuffer or such?

-#define BUFSIZE 1024
+#define BUFSIZE 4096

5/
+ strcat(buf, "WITH RECURSIVE expressions(schema, tbl, expr, typid) AS ("

I'm not an expert in writing complex SQL queries, so I asked Claude
Code to optimize and it came up with the suggestions [1]. Could you
check once?

6/ Just thinking if we also need to emit a warning in dryrun mode to
close the gaps. Something like: pg_log_warning("Verify that the OIDs
of the large objects to be removed are not stored in other data
columns or outside the database."); after:Would remove X large objects
from database "xxxx".

> 0002 - adds documentation mentioning the "data loss" and to nudge
> users to use vacuumlo.

1/ Nit: Use "Large object OIDs" instead of references for consistency?

+ Large object references stored in columns of other types, such as

2/ Suggesting to create an extension might be too much here. Can we
instead use oid type or lo like that? Here's my wording [2] in case it
helps.

+ resulting in data loss. Using the <type>lo</type> type
+ from the <xref linkend="lo"/> extension is recommended for columns that
+ hold large object references, as it allows

[1]
- Pre-filter the non-recursive part of the CTE so only columns whose
type can begin a path to `oid` enter the recursion: join `pg_type` and
add `AND (t.oid = 'pg_catalog.oid'::regtype OR t.typtype IN ('d','c')
OR (t.typelem <> 0 AND t.typlen = -1))`. Otherwise every `text` or
`int` column is seeded and probes `pg_type` three times before
terminating. On a 6040-column catalog this took the query from 105 ms
to 22 ms, same output.
- Drop `OR typid IN (SELECT oid FROM pg_type WHERE typname = 'lo')`:
`lo` is a domain over `oid`, so the domain branch already finds it,
and the clause makes the run fail on any unrelated type named `lo`. It
also duplicates every `lo` and `lo[]` column, so each such table is
scanned twice.
- Use `SELECT DISTINCT`: drops the remaining duplicate rows, saving
one scan of the table per duplicate.
- Write `'pg_catalog.oid'::regtype` instead of `26`: for readability.
- Drop `AND t.typrelid != 0`: implied by `typtype = 'c'`.
- Write `pg_catalog.quote_ident(...)` instead of relying on the
`search_path` set earlier.

[2]
<caution>
<para>
Only <type>oid</type> and <type>lo</type> data columns are considered. An LO
whose OID is stored in a data column of some other type, such as
<type>text</type> or <type>bigint</type>, is considered orphaned and will be
removed, resulting in data loss. Store LO OIDs in an <type>oid</type> or
<type>lo</type> data column. The <type>lo</type> type comes from the
<xref linkend="lo"/> module, which also provides the
<function>lo_manage</function> trigger to unlink an LO when the row
referencing it is deleted or updated.
</para>
</caution>

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com