brin_summarize_new_values error checking
In reviewing one of my patches[1]/messages/by-id/CAHGQGwH=M1BAEJPQdpJjCneqwg8Xa+P8SB+ZsvhVwH6gL2J46w@mail.gmail.com, Fujii-san has pointed out that I
didn't include checks for being in recovery, or for working on another
backend's temporary index.
I think that brin_summarize_new_values in 9.5.0 commits those same
sins. In its case, I don't think those are critical, as they just
result in getting less specific error messages that one might hope
for, rather than something worse like a panic.
But still, we might want to address them.
Cheers,
Jeff
[1]: /messages/by-id/CAHGQGwH=M1BAEJPQdpJjCneqwg8Xa+P8SB+ZsvhVwH6gL2J46w@mail.gmail.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jan 25, 2016 at 4:03 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
In reviewing one of my patches[1], Fujii-san has pointed out that I
didn't include checks for being in recovery, or for working on another
backend's temporary index.I think that brin_summarize_new_values in 9.5.0 commits those same
sins. In its case, I don't think those are critical, as they just
result in getting less specific error messages that one might hope
for, rather than something worse like a panic.But still, we might want to address them.
Agreed to add those checks. Also, I think we should document that
index maintenance functions cannot be executed during recovery.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jan 27, 2016 at 11:56 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Mon, Jan 25, 2016 at 4:03 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
In reviewing one of my patches[1], Fujii-san has pointed out that I
didn't include checks for being in recovery, or for working on another
backend's temporary index.I think that brin_summarize_new_values in 9.5.0 commits those same
sins. In its case, I don't think those are critical, as they just
result in getting less specific error messages that one might hope
for, rather than something worse like a panic.But still, we might want to address them.
Agreed to add those checks.
Attached patch does this.
Regards,
--
Fujii Masao
Attachments:
add_error_checks_to_brin_summarize_new_values_v1.patchtext/x-patch; charset=US-ASCII; name=add_error_checks_to_brin_summarize_new_values_v1.patchDownload
*** a/src/backend/access/brin/brin.c
--- b/src/backend/access/brin/brin.c
***************
*** 795,800 **** brin_summarize_new_values(PG_FUNCTION_ARGS)
--- 795,806 ----
Relation heapRel;
double numSummarized = 0;
+ if (RecoveryInProgress())
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("recovery is in progress"),
+ errhint("BRIN page ranges cannot be summarized during recovery.")));
+
/*
* We must lock table before index to avoid deadlocks. However, if the
* passed indexoid isn't an index then IndexGetRelation() will fail.
***************
*** 817,822 **** brin_summarize_new_values(PG_FUNCTION_ARGS)
--- 823,838 ----
errmsg("\"%s\" is not a BRIN index",
RelationGetRelationName(indexRel))));
+ /*
+ * Reject attempts to read non-local temporary relations; we would be
+ * likely to get wrong data since we have no visibility into the owning
+ * session's local buffers.
+ */
+ if (RELATION_IS_OTHER_TEMP(indexRel))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot access temporary indexes of other sessions")));
+
/* User must own the index (comparable to privileges needed for VACUUM) */
if (!pg_class_ownercheck(indexoid, GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
Fujii Masao wrote:
On Wed, Jan 27, 2016 at 11:56 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Mon, Jan 25, 2016 at 4:03 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
In reviewing one of my patches[1], Fujii-san has pointed out that I
didn't include checks for being in recovery, or for working on another
backend's temporary index.I think that brin_summarize_new_values in 9.5.0 commits those same
sins. In its case, I don't think those are critical, as they just
result in getting less specific error messages that one might hope
for, rather than something worse like a panic.But still, we might want to address them.
Agreed to add those checks.
Attached patch does this.
Uh, I just realized you never applied this patch ... Will you do so now?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers