| From: | Ajit Awekar <ajitpostgres(at)gmail(dot)com> |
|---|---|
| To: | Andrew Dunstan <andrew(at)dunslane(dot)net> |
| Cc: | Aleksander Alekseev <aleksander(at)tigerdata(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Junwang Zhao <zhjwpku(at)gmail(dot)com>, Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com>, Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>, Julien Tachoires <julien(at)tachoires(dot)me> |
| Subject: | Re: Allow table AMs to define their own reloptions |
| Date: | 2026-09-30 08:35:52 |
| Message-ID: | CAER375NyucpO_f735Q69PaHuJvNZiqrsJJQxrNy30Tmw-vE6Nw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
I tested v8 and found below two issues.
1. SET before RESET. Final state (heap, fillfactor=50) is valid. but
resulting in an error ERROR: unrecognized parameter "option_int". However
Reset before set works as expected.
postgres=# CREATE EXTENSION dummy_table_am;
CREATE EXTENSION
postgres=# CREATE TABLE x (a int);
CREATE TABLE
postgres=# ALTER TABLE x SET ACCESS METHOD dummy_table_am, SET (option_int
= 25);
ALTER TABLE
postgres=# SELECT reloptions FROM pg_class WHERE oid = 'x'::regclass;
reloptions
-----------------
{option_int=25}
(1 row)
postgres=# ALTER TABLE x SET ACCESS METHOD heap, SET (fillfactor = 50),
RESET (option_int);
ERROR: unrecognized parameter "option_int"
postgres=# SELECT (SELECT amname FROM pg_am WHERE oid = relam) AS amname,
reloptions FROM pg_class WHERE oid = 'x'::regclass;
amname | reloptions
----------------+-----------------
dummy_table_am | {option_int=25}
(1 row)
postgres=# ALTER TABLE x SET ACCESS METHOD heap, RESET (option_int), SET
(fillfactor = 50);
ALTER TABLE
postgres=# SELECT (SELECT amname FROM pg_am WHERE oid = relam) AS amname,
reloptions FROM pg_class WHERE oid = 'x'::regclass;
amname | reloptions
--------+-----------------
heap | {fillfactor=50}
(1 row)
2.The current dummy_table_am always embeds StdRdOptions, so it cannot
catch this. For this issue make dummy_table_am's struct start with
{int32 vl_len_; int pad1; int pad2; int option_int;}, keep only the
option_int registration, dt_relopt_tab[1], and set
has_std_options_prefix = false.
postgres=# CREATE EXTENSION dummy_table_am;
CREATE EXTENSION
postgres=# CREATE TABLE t0 (a int, b text) USING dummy_table_am WITH
(option_int = 0);
ERROR: unexpected toast_value_type value 0
postgres=# CREATE TABLE t2 (a int, b text) USING dummy_table_am WITH
(option_int = 2);
CREATE TABLE
postgres=# SELECT atttypid::regtype AS chunk_id_type FROM pg_attribute
WHERE attrelid = (SELECT reltoastrelid FROM pg_class WHERE oid =
't2'::regclass) AND attname = 'chunk_id';
chunk_id_type
---------------
oid8
(1 row)
postgres=# CREATE TABLE tn (a int, b text) USING dummy_table_am;
CREATE TABLE
postgres=# SELECT atttypid::regtype AS chunk_id_type FROM pg_attribute
WHERE attrelid = (SELECT reltoastrelid FROM pg_class WHERE oid =
'tn'::regclass) AND attname = 'chunk_id';
chunk_id_type
---------------
oid
(1 row)
I think RelationGetToastValueType() is not guarded by
RelationHasStdRdOptions() causing this issue.
Possible fix for second issue:
#define RelationGetToastValueType(relation, defaulttarg) \
(RelationHasStdRdOptions(relation) ? \
((StdRdOptions *) (relation)->rd_options)->toast_value_type :
(defaulttarg))
Thanks & Best Regards,
Ajit
On Mon, 28 Sept 2026 at 18:53, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 2026-09-08 Tu 8:34 AM, Ajit Awekar wrote:
>
> Hi all,
>
> I forgot to attach the TAM code used for repro. please find it attached
>
> Thanks & Best Regards,
> Ajit
>
> On Tue, 8 Sept 2026 at 15:33, Ajit Awekar <ajitpostgres(at)gmail(dot)com> wrote:
>
>> Hi hackers,
>>
>> I managed to get a crash with the patch. Below are the details
>>
>>
>> Repro:
>>
>> Any table AM that sets amoptions but leaves has_std_options_prefix false
>> and returns a bytea smaller than sizeof(StdRdOptions) will
>> crash on VACUUM of a table that has a toastable column.
>>
>> postgres=# CREATE EXTENSION tiny_table_am;
>> CREATE EXTENSION
>> postgres=# CREATE TABLE t_tiny (a int, b text) USING tiny_table_am WITH
>> (option_int = 7);
>> CREATE TABLE
>> postgres=# INSERT INTO t_tiny VALUES (1, repeat('x', 10000));
>> INSERT 0 1
>> postgres=# VACUUM t_tiny;
>> server closed the connection unexpectedly
>> This probably means the server terminated abnormally
>> before or while processing the request.
>> The connection to the server was lost. Attempting reset: Failed.
>> The connection to the server was lost. Attempting reset: Failed.
>>
>>
>> Root cause:
>> vacuum_rel() in has two places that read
>> rel->rd_options as a StdRdOptions to hand storage parameters down to the
>> relation's TOAST table. Only one of them was updated to use the new
>> RelationHasStdRdOptions() guard:
>>
>> ~line 2214 (correctly guarded):
>> relopts = merge_toast_reloptions(RelationHasStdRdOptions(rel) ?
>> (StdRdOptions *) rel->rd_options :
>> NULL,
>> params.main_relopts);
>>
>> ~line 2310-2312 (still just checks != NULL):
>> if (OidIsValid(toast_relid) && rel->rd_options)
>> {
>> memcpy(&relopts_copy, rel->rd_options, sizeof(StdRdOptions));
>> toast_vacuum_params.main_relopts = &relopts_copy;
>> }
>>
>>
>>
>>
>> Suggested fix
>> -------------
>> Same guard as the nearby, already-fixed call:
>>
>> --- a/src/backend/commands/vacuum.c
>> +++ b/src/backend/commands/vacuum.c
>> @@ -2307,9 +2307,8 @@ vacuum_rel(Oid relid, RangeVar *relation,
>> VacuumParams params,
>> * Hand our storage parameters down for the TOAST table to inherit.
>> Take
>> * a copy while we still have the relation open; the relcache entry
>> can go
>> * away once we close it.
>> */
>> - if (OidIsValid(toast_relid) && rel->rd_options)
>> + if (OidIsValid(toast_relid) && RelationHasStdRdOptions(rel))
>> {
>> memcpy(&relopts_copy, rel->rd_options, sizeof(StdRdOptions));
>> toast_vacuum_params.main_relopts = &relopts_copy;
>> }
>>
>>
>>>
> Thanks, this should be fixed in v8 attached.
>
>
> cheers
>
>
> andrew
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>
>
| From | Date | Subject | |
|---|---|---|---|
| Next Message | 2026-09-30 08:38:58 | WG: AW: Extract text from XML, pay attention to XML Entities | |
| Previous Message | Vlad Lesin | 2026-09-30 08:34:23 | Re: ReplicationSlotRelease() clobbers another backend's statusFlags entry |