public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Use correct macro to check compressed-ness in amcheck
4+ messages / 4 participants
[nested] [flat]
* [PATCH] Use correct macro to check compressed-ness in amcheck
@ 2022-05-17 07:19 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Kyotaro Horiguchi @ 2022-05-17 07:19 UTC (permalink / raw)
check_tuple_attribute uses VARATT_IS_COMPRESSED() for
varatt_external. This should be VARATT_EXTERNAL_IS_COMPRESSED()
instead. Remove the following cross check for data size since the
ycondition cannot be true.
---
contrib/amcheck/verify_heapam.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index c875f3e5a2..e488f5e234 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -1385,19 +1385,11 @@ check_tuple_attribute(HeapCheckContext *ctx)
toast_pointer.va_rawsize,
VARLENA_SIZE_LIMIT));
- if (VARATT_IS_COMPRESSED(&toast_pointer))
+ if (VARATT_EXTERNAL_IS_COMPRESSED(toast_pointer))
{
ToastCompressionId cmid;
bool valid = false;
- /* Compression should never expand the attribute */
- if (VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer) > toast_pointer.va_rawsize - VARHDRSZ)
- report_corruption(ctx,
- psprintf("toast value %u external size %u exceeds maximum expected for rawsize %d",
- toast_pointer.va_valueid,
- VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer),
- toast_pointer.va_rawsize));
-
/* Compressed attributes should have a valid compression method */
cmid = TOAST_COMPRESS_METHOD(&toast_pointer);
switch (cmid)
--
2.27.0
----Next_Part(Tue_May_17_16_27_19_2022_200)----
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Disallow altering invalidated replication slots
@ 2024-09-10 03:09 Peter Smith <[email protected]>
0 siblings, 2 replies; 4+ messages in thread
From: Peter Smith @ 2024-09-10 03:09 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
Hi, here are some review comments for patch v1.
======
Commit message
1.
ALTER_REPLICATION_SLOT on invalidated replication slots is unnecessary
as there is no way...
suggestion:
ALTER_REPLICATION_SLOT for invalid replication slots should not be
allowed because there is no way...
======
2. Missing docs update
Should this docs page [1] be updated to say ALTER_REPLICATION_SLOT is
not allowed for invalid slots?
======
src/backend/replication/slot.c
3.
+ if (MyReplicationSlot->data.invalidated != RS_INVAL_NONE)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot alter replication slot \"%s\"", name),
+ errdetail("This replication slot was invalidated due to \"%s\".",
+ SlotInvalidationCauses[MyReplicationSlot->data.invalidated]));
+
I thought including the reason "invalid" (e.g. "cannot alter invalid
replication slot \"%s\"") in the message might be better, but OTOH I
see the patch message is the same as an existing one. Maybe see what
others think.
======
src/test/recovery/t/035_standby_logical_decoding.pl
3.
There is already a comment about this test:
##################################################
# Recovery conflict: Invalidate conflicting slots, including in-use slots
# Scenario 1: hot_standby_feedback off and vacuum FULL
#
# In passing, ensure that replication slot stats are not removed when the
# active slot is invalidated.
##################################################
IMO we should update that "In passing..." sentence to something like:
In passing, ensure that replication slot stats are not removed when
the active slot is invalidated, and check that an error occurs when
attempting to alter the invalid slot.
======
[1] docs - https://www.postgresql.org/docs/devel/protocol-replication.html
Kind Regards,
Peter Smith.
Fujitsu Austalia
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Disallow altering invalidated replication slots
@ 2024-09-10 05:23 Amit Kapila <[email protected]>
parent: Peter Smith <[email protected]>
1 sibling, 0 replies; 4+ messages in thread
From: Amit Kapila @ 2024-09-10 05:23 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Sep 10, 2024 at 8:40 AM Peter Smith <[email protected]> wrote:
>
> Hi, here are some review comments for patch v1.
>
> ======
> Commit message
>
> 1.
> ALTER_REPLICATION_SLOT on invalidated replication slots is unnecessary
> as there is no way...
>
> suggestion:
> ALTER_REPLICATION_SLOT for invalid replication slots should not be
> allowed because there is no way...
>
> ======
> 2. Missing docs update
>
> Should this docs page [1] be updated to say ALTER_REPLICATION_SLOT is
> not allowed for invalid slots?
>
> ======
> src/backend/replication/slot.c
>
> 3.
> + if (MyReplicationSlot->data.invalidated != RS_INVAL_NONE)
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot alter replication slot \"%s\"", name),
> + errdetail("This replication slot was invalidated due to \"%s\".",
> + SlotInvalidationCauses[MyReplicationSlot->data.invalidated]));
> +
>
> I thought including the reason "invalid" (e.g. "cannot alter invalid
> replication slot \"%s\"") in the message might be better,
>
Agreed, I could see a similar case with a message ("cannot alter
invalid database \"%s\"") in the code. Additionally, we should also
include Shveta's suggestion to change the detailed message to other
similar messages in logical.c
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Disallow altering invalidated replication slots
@ 2024-09-10 17:54 Bharath Rupireddy <[email protected]>
parent: Peter Smith <[email protected]>
1 sibling, 0 replies; 4+ messages in thread
From: Bharath Rupireddy @ 2024-09-10 17:54 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
Hi,
Thanks for reviewing.
On Tue, Sep 10, 2024 at 8:40 AM Peter Smith <[email protected]> wrote:
>
> Commit message
>
> 1.
> ALTER_REPLICATION_SLOT on invalidated replication slots is unnecessary
> as there is no way...
>
> suggestion:
> ALTER_REPLICATION_SLOT for invalid replication slots should not be
> allowed because there is no way...
Modified.
> ======
> 2. Missing docs update
>
> Should this docs page [1] be updated to say ALTER_REPLICATION_SLOT is
> not allowed for invalid slots?
Haven't noticed for other ERROR cases in the docs, e.g. slots being
synced, temporary slots. Not sure if it's worth adding every ERROR
case to the docs.
> ======
> src/backend/replication/slot.c
>
> 3.
> + if (MyReplicationSlot->data.invalidated != RS_INVAL_NONE)
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot alter replication slot \"%s\"", name),
> + errdetail("This replication slot was invalidated due to \"%s\".",
> + SlotInvalidationCauses[MyReplicationSlot->data.invalidated]));
> +
>
> I thought including the reason "invalid" (e.g. "cannot alter invalid
> replication slot \"%s\"") in the message might be better, but OTOH I
> see the patch message is the same as an existing one. Maybe see what
> others think.
Changed.
> ======
> src/test/recovery/t/035_standby_logical_decoding.pl
>
> 3.
> There is already a comment about this test:
> ##################################################
> # Recovery conflict: Invalidate conflicting slots, including in-use slots
> # Scenario 1: hot_standby_feedback off and vacuum FULL
> #
> # In passing, ensure that replication slot stats are not removed when the
> # active slot is invalidated.
> ##################################################
>
> IMO we should update that "In passing..." sentence to something like:
>
> In passing, ensure that replication slot stats are not removed when
> the active slot is invalidated, and check that an error occurs when
> attempting to alter the invalid slot.
Added. But, keeping it closer to the test case doesn't hurt.
Please find the attached v2 patch also having Shveta's review comments
addressed.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
[application/octet-stream] v2-0001-Disallow-altering-invalidated-replication-slots.patch (3.0K, ../../CALj2ACVh8otuX5HS1bB5=ikVk4UNHCkOaw2CbqTv1-E4J82p=g@mail.gmail.com/2-v2-0001-Disallow-altering-invalidated-replication-slots.patch)
download | inline diff:
From 80f9ee78ad72675fb1e2e1d28f1cfe75a1d1e18b Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Tue, 10 Sep 2024 17:51:56 +0000
Subject: [PATCH v2] Disallow altering invalidated replication slots
ALTER_REPLICATION_SLOT for invalid replication slots should not be
allowed because there is no way to get back the invalidated
(logical) slot to working. This commit adds an error.
Author: Bharath Rupireddy
Reviewed-by: Peter Smith, shveta malik
Discussion: https://www.postgresql.org/message-id/CAA4eK1%2Bszcosq0nS109mMSxPWyNT1Q%3DUNYCJgXKYuCceaPS%2BhA%40mail.gmail.com
---
src/backend/replication/slot.c | 7 +++++++
src/test/recovery/t/035_standby_logical_decoding.pl | 12 +++++++++++-
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 0a03776156..6828100cf1 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -820,6 +820,13 @@ ReplicationSlotAlter(const char *name, const bool *failover,
errmsg("cannot use %s with a physical replication slot",
"ALTER_REPLICATION_SLOT"));
+ if (MyReplicationSlot->data.invalidated != RS_INVAL_NONE)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot alter invalid replication slot \"%s\"", name),
+ errdetail("This replication slot has been invalidated due to \"%s\".",
+ SlotInvalidationCauses[MyReplicationSlot->data.invalidated]));
+
if (RecoveryInProgress())
{
/*
diff --git a/src/test/recovery/t/035_standby_logical_decoding.pl b/src/test/recovery/t/035_standby_logical_decoding.pl
index 4185b803b2..d658fdf417 100644
--- a/src/test/recovery/t/035_standby_logical_decoding.pl
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -495,7 +495,8 @@ $node_subscriber->stop;
# Scenario 1: hot_standby_feedback off and vacuum FULL
#
# In passing, ensure that replication slot stats are not removed when the
-# active slot is invalidated.
+# active slot is invalidated, and check that an error occurs when
+# attempting to alter the invalid slot.
##################################################
# One way to produce recovery conflict is to create/drop a relation and
@@ -526,6 +527,15 @@ check_for_invalidation('vacuum_full_', 1, 'with vacuum FULL on pg_class');
# Verify reason for conflict is 'rows_removed' in pg_replication_slots
check_slots_conflict_reason('vacuum_full_', 'rows_removed');
+# Attempting to alter an invalidated slot should result in an error
+($result, $stdout, $stderr) = $node_standby->psql(
+ 'postgres',
+ qq[ALTER_REPLICATION_SLOT vacuum_full_inactiveslot (failover);],
+ replication => 'database');
+ok($stderr =~ /ERROR: cannot alter invalid replication slot "vacuum_full_inactiveslot"/ &&
+ $stderr =~ /DETAIL: This replication slot has been invalidated due to "rows_removed"./,
+ "invalidated slot cannot be altered");
+
# Ensure that replication slot stats are not removed after invalidation.
is( $node_standby->safe_psql(
'testdb',
--
2.43.0
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-09-10 17:54 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-05-17 07:19 [PATCH] Use correct macro to check compressed-ness in amcheck Kyotaro Horiguchi <[email protected]>
2024-09-10 03:09 Re: Disallow altering invalidated replication slots Peter Smith <[email protected]>
2024-09-10 05:23 ` Re: Disallow altering invalidated replication slots Amit Kapila <[email protected]>
2024-09-10 17:54 ` Re: Disallow altering invalidated replication slots Bharath Rupireddy <[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