On Tue, Apr 29, 2025 at 02:01:54PM -0400, Tom Lane wrote:
> I'm inclined to argue that it's a bug fix and therefore still in-scope
> for v18. The fact that we can't back-patch such a change is all the
> more reason to not let it slide another year.
Not on the RMT. I have looked at the patch, and
On Tue, Apr 29, 2025 at 02:01:54PM -0400, Tom Lane wrote:
> Nathan Bossart writes:
>> Thanks. I'll stash this away for July unless someone wants to argue that
>> it's fair game for v18. IMHO this isn't nearly urgent enough for that, and
>> the bugs will continue to exist on older major versions
Nathan Bossart writes:
> Thanks. I'll stash this away for July unless someone wants to argue that
> it's fair game for v18. IMHO this isn't nearly urgent enough for that, and
> the bugs will continue to exist on older major versions regardless.
I'm inclined to argue that it's a bug fix and ther
On Mon, Apr 28, 2025 at 06:19:19PM -0400, Tom Lane wrote:
> LGTM other than that nit.
Thanks. I'll stash this away for July unless someone wants to argue that
it's fair game for v18. IMHO this isn't nearly urgent enough for that, and
the bugs will continue to exist on older major versions regard
Nathan Bossart writes:
> On Mon, Apr 28, 2025 at 05:35:08PM -0400, Tom Lane wrote:
>> Possibly better idea: can we add something like
>> Assert(!OidIsValid(reltoastrelid)) in the code that is making this
>> assumption?
> Yeah, we could add something like that to replorigin_create() pretty
> easil
On Mon, Apr 28, 2025 at 05:35:08PM -0400, Tom Lane wrote:
> Possibly better idea: can we add something like
> Assert(!OidIsValid(reltoastrelid)) in the code that is making this
> assumption?
Yeah, we could add something like that to replorigin_create() pretty
easily. The comment for that would be
Nathan Bossart writes:
> On Wed, Apr 23, 2025 at 04:44:51PM -0500, Nathan Bossart wrote:
>> On Wed, Apr 23, 2025 at 05:33:41PM -0400, Tom Lane wrote:
>>> I don't see any comments in this patch that capture the real
>>> reason for removing pg_replication_origin's TOAST table,
>>> namely (IIUC) that
On Wed, Apr 23, 2025 at 04:44:51PM -0500, Nathan Bossart wrote:
> On Wed, Apr 23, 2025 at 05:33:41PM -0400, Tom Lane wrote:
>> I don't see any comments in this patch that capture the real
>> reason for removing pg_replication_origin's TOAST table,
>> namely (IIUC) that we'd like to be able to acces
On Wed, Apr 23, 2025 at 05:33:41PM -0400, Tom Lane wrote:
> Nathan Bossart writes:
>> Here is a new version of the patch with this change.
>
> I don't see any comments in this patch that capture the real
> reason for removing pg_replication_origin's TOAST table,
> namely (IIUC) that we'd like to
Nathan Bossart writes:
> Here is a new version of the patch with this change.
I don't see any comments in this patch that capture the real
reason for removing pg_replication_origin's TOAST table,
namely (IIUC) that we'd like to be able to access that catalog
without a snapshot. I think it's impo
On Tue, Apr 22, 2025 at 12:21:01PM -0500, Nathan Bossart wrote:
> On Tue, Apr 22, 2025 at 05:17:17PM +0530, Nisha Moond wrote:
>> 5) As Euler pointed out, logical replication already has a built-in
>> restriction for internally assigned origin names, limiting them to
>> NAMEDATALEN. Given that, onl
On Tue, Apr 22, 2025 at 05:17:17PM +0530, Nisha Moond wrote:
> Thanks for the patch! I tested it for functionality and here are a few
> comments:
Thank you for reviewing.
> 1) While testing, I noticed an unexpected behavior with the new 512
> byte length restriction in place. Since there´s no ex
On Mon, Apr 21, 2025 at 9:26 PM Nathan Bossart wrote:
>
> Apparently replication origins in tests are supposed to be prefixed with
> "regress_". Here is a new patch with that fixed.
>
Hi Nathan,
Thanks for the patch! I tested it for functionality and here are a few comments:
1) While testing, I
Apparently replication origins in tests are supposed to be prefixed with
"regress_". Here is a new patch with that fixed.
--
nathan
>From fa6a8a5a155a7beb50e54bfe42c81be26ffd428a Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Mon, 21 Apr 2025 10:53:15 -0500
Subject: [PATCH v3 1/1] Remove p
On Wed, Apr 09, 2025 at 08:54:21PM -0300, Euler Taveira wrote:
> LGTM. I have a few suggestions.
Thanks for reviewing.
> + /*
> +* To avoid needing a TOAST table for pg_replication_origin, we restrict
> +* replication origin names to 512 bytes. This should be more than enough
> +*
On Wed, Apr 9, 2025, at 4:05 PM, Nathan Bossart wrote:
> On Wed, Apr 09, 2025 at 01:20:29PM +0900, Michael Paquier wrote:
> > I guess that's the consensus, then. No objections to the removal here.
>
> That would look like the attached. There are still a couple of other known
> TOAST snapshot iss
On Wed, Apr 09, 2025 at 01:20:29PM +0900, Michael Paquier wrote:
> I guess that's the consensus, then. No objections to the removal here.
That would look like the attached. There are still a couple of other known
TOAST snapshot issues I need to revisit, but this sidesteps a few of them.
But this
On Tue, Apr 08, 2025 at 11:21:31PM -0300, Euler Taveira wrote:
> The logical replication creates origin names as pg_SUBOID_RELID or pg_SUBOID.
> It means the maximum origin name is 24. This limited origin name also applies
> to pglogical that limits the name to 54 IIRC. I think that covers the majo
On Tue, Apr 8, 2025, at 5:25 PM, Nathan Bossart wrote:
> On Tue, Apr 08, 2025 at 04:44:02PM +0530, Amit Kapila wrote:
> > On Fri, Apr 4, 2025 at 7:58 PM Nathan Bossart
> > wrote:
> >> On Fri, Apr 04, 2025 at 05:16:43PM +0530, Amit Kapila wrote:
> >> > Can we dodge adding this push call if we rest
On Tue, Apr 08, 2025 at 04:44:02PM +0530, Amit Kapila wrote:
> On Fri, Apr 4, 2025 at 7:58 PM Nathan Bossart
> wrote:
>> On Fri, Apr 04, 2025 at 05:16:43PM +0530, Amit Kapila wrote:
>> > Can we dodge adding this push call if we restrict the length of the
>> > origin name to some reasonable limit
On Fri, Apr 4, 2025 at 7:58 PM Nathan Bossart wrote:
>
> On Fri, Apr 04, 2025 at 05:16:43PM +0530, Amit Kapila wrote:
> > Can we dodge adding this push call if we restrict the length of the
> > origin name to some reasonable limit like 256 or 512 and avoid the
> > need of toast altogether?
>
> We
On Wed, Nov 27, 2024 at 9:29 PM Nathan Bossart wrote:
>
> On Wed, Nov 27, 2024 at 03:20:19PM +0900, Michael Paquier wrote:
> > On Mon, Nov 25, 2024 at 01:29:31PM -0600, Nathan Bossart wrote:
> >> Or we could just enforce that you have an active snapshot whenever you
> >> modify a catalog with a TO
On Fri, Apr 04, 2025 at 05:16:43PM +0530, Amit Kapila wrote:
> Can we dodge adding this push call if we restrict the length of the
> origin name to some reasonable limit like 256 or 512 and avoid the
> need of toast altogether?
We did consider just removing pg_replication_origin's TOAST table earl
On Wed, 27 Nov 2024 at 21:29, Nathan Bossart wrote:
>
> On Wed, Nov 27, 2024 at 03:20:19PM +0900, Michael Paquier wrote:
> > On Mon, Nov 25, 2024 at 01:29:31PM -0600, Nathan Bossart wrote:
> >> Or we could just enforce that you have an active snapshot whenever you
> >> modify a catalog with a TOAS
On Wed, Nov 27, 2024 at 03:20:19PM +0900, Michael Paquier wrote:
> On Mon, Nov 25, 2024 at 01:29:31PM -0600, Nathan Bossart wrote:
>> Or we could just enforce that you have an active snapshot whenever you
>> modify a catalog with a TOAST table. That's simpler, but it requires extra
>> work in some
On Mon, Nov 25, 2024 at 01:29:31PM -0600, Nathan Bossart wrote:
> Or we could just enforce that you have an active snapshot whenever you
> modify a catalog with a TOAST table. That's simpler, but it requires extra
> work in some paths (and probably comments to point out that we're only
> pushing a
On Thu, Oct 31, 2024 at 08:45:15AM +0900, Michael Paquier wrote:
> On Wed, Oct 30, 2024 at 03:54:32PM -0500, Nathan Bossart wrote:
>> I'll manage. 0001 was a doozy to back-patch, and this obviously isn't a
>> terribly pressing issue, so I plan to wait until after the November minor
>> release to a
On Wed, Oct 30, 2024 at 03:54:32PM -0500, Nathan Bossart wrote:
> I'll manage. 0001 was a doozy to back-patch, and this obviously isn't a
> terribly pressing issue, so I plan to wait until after the November minor
> release to apply this.
Okay by me.
> I'm having second thoughts on 0002, so I'll
On Fri, Oct 18, 2024 at 08:08:55AM +0900, Michael Paquier wrote:
> 0002 and 0003 look sane enough to me as shaped. I'd need to spend a
> few more hours on 0001 if I were to do a second round on it, but you
> don't really need a second opinion, do you? :D
I'll manage. 0001 was a doozy to back-pa
On Wed, Oct 16, 2024 at 10:54:45AM -0500, Nathan Bossart wrote:
> Here is a new version of the patch set with this #ifdef added. I plan to
> give each of the code paths adjusted by 0001 a closer look, but otherwise
> I'm hoping to commit these soon.
0002 and 0003 look sane enough to me as shaped.
On Wed, Oct 16, 2024 at 10:24:32AM +0900, Michael Paquier wrote:
> On Tue, Oct 15, 2024 at 08:20:17PM -0500, Nathan Bossart wrote:
>> I assume all of this will get compiled out in non-USE_ASSERT_CHECKING
>> builds as-is, but I see no problem with surrounding it with an #ifdef to be
>> sure.
>
> Ye
On Tue, Oct 15, 2024 at 08:20:17PM -0500, Nathan Bossart wrote:
> On Wed, Oct 16, 2024 at 09:12:31AM +0900, Michael Paquier wrote:
> > - if (!ctx->rel->rd_rel->reltoastrelid)
> > + if (!OidIsValid(RelationGetToastRelid(ctx->rel)))
> >
> > This set of diffs in 0002 is a nice cleanup. I'd wish
On Wed, Oct 16, 2024 at 09:12:31AM +0900, Michael Paquier wrote:
> - if (!ctx->rel->rd_rel->reltoastrelid)
> + if (!OidIsValid(RelationGetToastRelid(ctx->rel)))
>
> This set of diffs in 0002 is a nice cleanup. I'd wish for relying
> less on zero comparitons when assuming that InvalidOid is in
On Mon, Oct 14, 2024 at 03:02:22PM -0500, Nathan Bossart wrote:
> Here is a reorganized patch set. 0001 would be back-patched, but the
> others would only be applied to v18.
Right.
- if (!ctx->rel->rd_rel->reltoastrelid)
+ if (!OidIsValid(RelationGetToastRelid(ctx->rel)))
This set of diffs
On Tue, Oct 08, 2024 at 01:50:52PM -0500, Nathan Bossart wrote:
> 0002 could probably use some more commentary, but otherwise I think it is
> in decent shape. You (Michael) seem to be implying that I should
> back-patch the actual fixes and only apply the new assertions to v18. Am I
> understandi
On Wed, Sep 25, 2024 at 01:05:26PM +0900, Michael Paquier wrote:
> On Tue, Sep 24, 2024 at 02:26:08PM -0500, Nathan Bossart wrote:
>> So... maybe we
>> should just remove pg_replication_origin's TOAST table instead...
>
> I'd rather keep it, FWIW. Contrary to pg_authid it does not imply
> problem
On Tue, Sep 24, 2024 at 02:26:08PM -0500, Nathan Bossart wrote:
> On Tue, Sep 24, 2024 at 01:21:45PM +0900, Michael Paquier wrote:
>> On Mon, Sep 23, 2024 at 10:50:21AM -0500, Nathan Bossart wrote:
>>> I carefully inspected all the code paths this patch touches, and I think
>>> I've got all the det
On Tue, Sep 24, 2024 at 02:26:08PM -0500, Nathan Bossart wrote:
> I gave this a try and, unsurprisingly, found a bunch of other problems. I
> hastily hacked together the attached patch that should fix all of them, but
> I'd still like to comb through the code a bit more. The three catalogs
> with
On Tue, Sep 24, 2024 at 01:21:45PM +0900, Michael Paquier wrote:
> On Mon, Sep 23, 2024 at 10:50:21AM -0500, Nathan Bossart wrote:
>> I carefully inspected all the code paths this patch touches, and I think
>> I've got all the details right, but I would be grateful if someone else
>> could take a l
On Mon, Sep 23, 2024 at 10:50:21AM -0500, Nathan Bossart wrote:
> I carefully inspected all the code paths this patch touches, and I think
> I've got all the details right, but I would be grateful if someone else
> could take a look.
No objections from here with putting the snapshots pops and push
On Mon, Sep 23, 2024 at 04:00:00PM +0300, Alexander Lakhin wrote:
> I tested it with two code modifications (1st is to make each created
> expression index TOASTed (by prepending 1M of spaces to the indexeprs
> value) and 2nd to make each created index an expression index (by
> modifying index_elem
Hello Nathan,
20.09.2024 19:51, Nathan Bossart wrote:
Here's a (probably naive) attempt at fixing these, too. I'll give each
path a closer look once it feels like we've identified all the bugs.
Thank you for the updated patch!
I tested it with two code modifications (1st is to make each crea
On Fri, Sep 20, 2024 at 07:00:00AM +0300, Alexander Lakhin wrote:
> I've found another two paths to reach that condition:
> CREATE INDEX CONCURRENTLY ON def (vec_quantizer(id, :'b'));
> ERROR: cannot fetch toast data without an active snapshot
>
> REINDEX INDEX CONCURRENTLY def_vec_quantizer_idx;
On Fri, Sep 20, 2024 at 08:16:24AM +0900, Michael Paquier wrote:
> Perhaps the reason why these snapshots are pushed should be documented
> with a comment?
Definitely. I'll add those once we are more confident that we've
identified all the bugs.
--
nathan
Hello Nathan,
19.09.2024 21:36, Nathan Bossart wrote:
On Thu, Sep 19, 2024 at 12:00:00PM +0300, Alexander Lakhin wrote:
completed with:
DROP INDEX CONCURRENTLY def_vec_quantizer_idx;
triggers an assertion failure:
TRAP: failed Assert("HaveRegisteredOrActiveSnapshot()"), File:
"toast_internals
On Thu, Sep 19, 2024 at 01:36:36PM -0500, Nathan Bossart wrote:
> + PushActiveSnapshot(GetTransactionSnapshot());
>
> /*
>* Now we must wait until no running transaction could be using
> the
> @@ -2283,8 +2284,10 @@ index_drop(Oid indexId, bool concurren
On Thu, Sep 19, 2024 at 12:00:00PM +0300, Alexander Lakhin wrote:
> I've discovered that Jonathan's initial script:
> CREATE TABLE def (id int);
> SELECT array_agg(n) b FROM generate_series(1,10_000) n \gset
> CREATE OR REPLACE FUNCTION vec_quantizer (a int, b int[]) RETURNS bool
> AS $$ SELECT tru
Hello Nathan,
18.09.2024 22:52, Nathan Bossart wrote:
Committed. I waffled on whether to add a test for system indexes that used
pg_index's varlena columns, but I ended up leaving it out. I've attached
it here in case anyone thinks we should add it.
I've discovered that Jonathan's initial sc
On Wed, Sep 18, 2024 at 10:54:56AM -0400, Tom Lane wrote:
> Nathan Bossart writes:
>> Any objections to committing this?
>
> Nope.
Committed. I waffled on whether to add a test for system indexes that used
pg_index's varlena columns, but I ended up leaving it out. I've attached
it here in case
Nathan Bossart writes:
> On Wed, Sep 04, 2024 at 03:20:33PM -0400, Jonathan S. Katz wrote:
>> On 9/4/24 3:08 PM, Tom Lane wrote:
>>> Nathan Bossart writes:
Thanks to commit 96cdeae, only a few catalogs remain that are missing TOAST
tables: pg_attribute, pg_class, pg_index, pg_largeobjec
On Wed, Sep 04, 2024 at 03:20:33PM -0400, Jonathan S. Katz wrote:
> On 9/4/24 3:08 PM, Tom Lane wrote:
>> Nathan Bossart writes:
>> > Thanks to commit 96cdeae, only a few catalogs remain that are missing TOAST
>> > tables: pg_attribute, pg_class, pg_index, pg_largeobject, and
>> > pg_largeobject_m
On 9/4/24 3:08 PM, Tom Lane wrote:
Nathan Bossart writes:
Thanks to commit 96cdeae, only a few catalogs remain that are missing TOAST
tables: pg_attribute, pg_class, pg_index, pg_largeobject, and
pg_largeobject_metadata. I've attached a short patch to add one for
pg_index, which resolves the i
Nathan Bossart writes:
> Thanks to commit 96cdeae, only a few catalogs remain that are missing TOAST
> tables: pg_attribute, pg_class, pg_index, pg_largeobject, and
> pg_largeobject_metadata. I've attached a short patch to add one for
> pg_index, which resolves the issue cited here. This passes
On Tue, Sep 03, 2024 at 12:35:42PM -0400, Jonathan S. Katz wrote:
> However, I ran into the issue in[1], where pg_index was identified as
> catalog that is missing a toast table, even though `indexprs` is marked for
> extended storage. Providing a very simple reproducer in psql below:
Thanks to co
Hi,
I ran into an issue (previously discussed[1]; quoting Andres out of
context that not addressing it then would "[a]ll but guarantee that
we'll have this discussion again"[2]) when trying to build a very large
expression index that did not fit within the page boundary. The
real-world use ca
55 matches
Mail list logo
