public inbox for [email protected]
help / color / mirror / Atom feedRe: commitfest.postgresql.org is no longer fit for purpose
3+ messages / 3 participants
[nested] [flat]
* Re: commitfest.postgresql.org is no longer fit for purpose
@ 2024-05-24 21:23 Tomas Vondra <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Tomas Vondra @ 2024-05-24 21:23 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Joe Conway <[email protected]>; +Cc: Robert Haas <[email protected]>; Dmitry Dolgov <[email protected]>; pgsql-hackers
On 5/24/24 22:44, Tom Lane wrote:
> Joe Conway <[email protected]> writes:
>> On 5/24/24 15:45, Tom Lane wrote:
>>> I was *not* proposing doing a regular review, unless of course
>>> somebody really wants to do that. What I am thinking about is
>>> suggesting how to make progress on patches that are stuck, or in some
>>> cases delivering the bad news that this patch seems unlikely to ever
>>> get accepted and it's time to cut our losses. (Patches that seem to
>>> be moving along in good order probably don't need any attention in
>>> this process, beyond determining that that's the case.) That's why
>>> I think we need some senior people doing this, as their opinions are
>>> more likely to be taken seriously.
>
>> Maybe do a FOSDEM-style dev meeting with triage review at PG.EU would at
>> least move us forward? Granted it is less early and perhaps less often
>> than the thread seems to indicate, but has been tossed around before and
>> seems doable.
>
> Perhaps. The throughput of an N-person meeting is (at least) a factor
> of N less than the same N people looking at patches individually.
> On the other hand, the consensus of a meeting is more likely to be
> taken seriously than a single person's opinion, senior or not.
> So it could work, but I think we'd need some prefiltering so that
> the meeting only spends time on those patches already identified as
> needing help.
>
I personally don't think the FOSDEM triage is a very productive use of
our time - we go through patches top to bottom, often with little idea
what the current state of the patch is. We always ran out of time after
looking at maybe 1/10 of the list.
Having an in-person discussion about patches would be good, but I think
we should split the meeting into much smaller groups for that, each
looking at a different subset. And maybe it should be determined in
advance, so that people can look at those patches in advance ...
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
@ 2024-05-24 21:38 Tom Lane <[email protected]>
parent: Tomas Vondra <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Tom Lane @ 2024-05-24 21:38 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Joe Conway <[email protected]>; Robert Haas <[email protected]>; Dmitry Dolgov <[email protected]>; pgsql-hackers
Tomas Vondra <[email protected]> writes:
> I personally don't think the FOSDEM triage is a very productive use of
> our time - we go through patches top to bottom, often with little idea
> what the current state of the patch is. We always ran out of time after
> looking at maybe 1/10 of the list.
> Having an in-person discussion about patches would be good, but I think
> we should split the meeting into much smaller groups for that, each
> looking at a different subset. And maybe it should be determined in
> advance, so that people can look at those patches in advance ...
Yeah, subgroups of 3 or 4 people sounds about right. And definitely
some advance looking to see which patches need discussion.
regards, tom lane
^ permalink raw reply [nested|flat] 3+ messages in thread
* [PATCH] Copy the relevant pg_attrdef catalog entries for the transient relation.
@ 2026-06-22 07:34 Antonin Houska <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Antonin Houska @ 2026-06-22 07:34 UTC (permalink / raw)
The default values may be needed by the executor when processing the
concurrent data changes. In particular, ExecInsertIndexTuples() needs it when
determining the value of the 'indexUnchanged' hint for the index AM.
Like in copy_index_constraints(), we make the new catalog entries dependent on
the transient relation, so they are dropped along with it automatically.
---
src/backend/commands/repack.c | 97 +++++++++++++++++++
.../injection_points/specs/repack.spec | 3 +-
2 files changed, 99 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 4d177c868bb..725efa236bc 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -48,6 +48,7 @@
#include "catalog/namespace.h"
#include "catalog/objectaccess.h"
#include "catalog/pg_am.h"
+#include "catalog/pg_attrdef.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_inherits.h"
#include "catalog/toasting.h"
@@ -204,6 +205,7 @@ static void rebuild_relation_finish_concurrent(Relation NewHeap, Relation OldHea
static List *build_new_indexes(Relation NewHeap, Relation OldHeap, List *OldIndexes);
static void copy_index_constraints(Relation old_index, Oid new_index_id,
Oid new_heap_id);
+static void copy_attribute_defaults(Relation old_heap, Relation new_heap);
static Relation process_single_relation(RepackStmt *stmt,
LOCKMODE lockmode,
bool isTopLevel,
@@ -1083,6 +1085,13 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose,
Assert(CheckRelationOidLockedByMe(OIDNewHeap, AccessExclusiveLock, false));
NewHeap = table_open(OIDNewHeap, NoLock);
+ /*
+ * Copy attribute defaults - the executor may need them, in order to
+ * process the concurrent data changes. In particular, this is related to
+ * ExecInsertIndexTuples().
+ */
+ copy_attribute_defaults(OldHeap, NewHeap);
+
/* Copy the heap data into the new table in the desired order */
copy_table_data(NewHeap, OldHeap, index, snapshot, verbose,
&swap_toast_by_content, &frozenXid, &cutoffMulti);
@@ -3434,6 +3443,94 @@ copy_index_constraints(Relation old_index, Oid new_index_id, Oid new_heap_id)
CommandCounterIncrement();
}
+/*
+ * Create a transient copy of attribute defaults for the transient table.
+ *
+ * Like above, the executor needs information on attribute defaults. Once the
+ * repacking is finished, the catalog entries we create here are dropped.
+ */
+static void
+copy_attribute_defaults(Relation old_heap, Relation new_heap)
+{
+ Oid old_heap_id = RelationGetRelid(old_heap);
+ Oid new_heap_id = RelationGetRelid(new_heap);
+ ScanKeyData skey;
+ Relation rel;
+ Relation att_rel = NULL;
+ TupleDesc desc;
+ SysScanDesc scan;
+ HeapTuple tup;
+ ObjectAddress objrel;
+
+ rel = table_open(AttrDefaultRelationId, RowExclusiveLock);
+ ObjectAddressSet(objrel, RelationRelationId, new_heap_id);
+
+ ScanKeyInit(&skey,
+ Anum_pg_attrdef_adrelid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(old_heap_id));
+ scan = systable_beginscan(rel, AttrDefaultIndexId, true,
+ NULL, 1, &skey);
+ desc = RelationGetDescr(rel);
+ while (HeapTupleIsValid(tup = systable_getnext(scan)))
+ {
+ Form_pg_attrdef adform;
+ Oid oid;
+ Datum values[Natts_pg_attrdef] = {0};
+ bool nulls[Natts_pg_attrdef] = {0};
+ bool replaces[Natts_pg_attrdef] = {0};
+ HeapTuple new_tup, att_tup, att_new_tup;
+ ObjectAddress objad;
+ Datum att_values[Natts_pg_attribute] = {0};
+ bool att_nulls[Natts_pg_attribute] = {0};
+ bool att_replaces[Natts_pg_attribute] = {0};
+
+ adform = (Form_pg_attrdef) GETSTRUCT(tup);
+ Assert(adform->adrelid == old_heap_id);
+
+ oid = GetNewOidWithIndex(rel, AttrDefaultOidIndexId,
+ Anum_pg_attrdef_oid);
+ values[Anum_pg_attrdef_oid - 1] = ObjectIdGetDatum(oid);
+ replaces[Anum_pg_attrdef_oid - 1] = true;
+ values[Anum_pg_attrdef_adrelid - 1] = ObjectIdGetDatum(new_heap_id);
+ replaces[Anum_pg_attrdef_adrelid - 1] = true;
+
+ new_tup = heap_modify_tuple(tup, desc, values, nulls, replaces);
+
+ /* Insert it into the catalog. */
+ CatalogTupleInsert(rel, new_tup);
+
+ /* Create a dependency so it's removed when we drop the new heap. */
+ ObjectAddressSet(objad, AttrDefaultRelationId, oid);
+ recordDependencyOn(&objad, &objrel, DEPENDENCY_AUTO);
+
+ /* Set atthasdef - new heap has it cleared. */
+ att_tup = SearchSysCache2(ATTNUM,
+ ObjectIdGetDatum(new_heap_id),
+ ObjectIdGetDatum(adform->adnum));
+ if (!HeapTupleIsValid(att_tup))
+ elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+ adform->adnum, new_heap_id);
+
+ att_values[Anum_pg_attribute_atthasdef - 1] = BoolGetDatum(true);
+ att_replaces[Anum_pg_attribute_atthasdef - 1] = true;
+
+ if (att_rel == NULL)
+ att_rel = table_open(AttributeRelationId, RowExclusiveLock);
+ att_new_tup = heap_modify_tuple(att_tup, RelationGetDescr(att_rel),
+ att_values, att_nulls, att_replaces);
+ ReleaseSysCache(att_tup);
+ CatalogTupleUpdate(att_rel, &att_new_tup->t_self, att_new_tup);
+ }
+ systable_endscan(scan);
+
+ table_close(rel, RowExclusiveLock);
+ if (att_rel)
+ table_close(att_rel, RowExclusiveLock);
+
+ CommandCounterIncrement();
+}
+
/*
* Try to start a background worker to perform logical decoding of data
* changes applied to relation while REPACK CONCURRENTLY is copying its
diff --git a/src/test/modules/injection_points/specs/repack.spec b/src/test/modules/injection_points/specs/repack.spec
index d727a9b056b..7896d1456ad 100644
--- a/src/test/modules/injection_points/specs/repack.spec
+++ b/src/test/modules/injection_points/specs/repack.spec
@@ -3,7 +3,8 @@ setup
{
CREATE EXTENSION injection_points;
- CREATE TABLE repack_test(i int PRIMARY KEY, j int);
+ CREATE TABLE repack_test(i int PRIMARY KEY, j int,
+ k int GENERATED ALWAYS AS (j * 2) STORED);
INSERT INTO repack_test(i, j) VALUES (1, 1), (2, 2), (3, 3), (4, 4);
CREATE TABLE relfilenodes(node oid);
--
2.52.0
--=-=-=--
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-06-22 07:34 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-05-24 21:23 Re: commitfest.postgresql.org is no longer fit for purpose Tomas Vondra <[email protected]>
2024-05-24 21:38 ` Tom Lane <[email protected]>
2026-06-22 07:34 [PATCH] Copy the relevant pg_attrdef catalog entries for the transient relation. Antonin Houska <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox