Re: [HACKERS] brin_summarize_new_values error checking
Fujii Masao wrote: > On Wed, Jan 27, 2016 at 11:56 AM, Fujii Masaowrote: > > On Mon, Jan 25, 2016 at 4:03 PM, Jeff Janes 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 Herrerahttps://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
Re: [HACKERS] brin_summarize_new_values error checking
On Wed, Jan 27, 2016 at 11:56 AM, Fujii Masaowrote: > On Mon, Jan 25, 2016 at 4:03 PM, Jeff Janes 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 *** 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, -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] brin_summarize_new_values error checking
On Mon, Jan 25, 2016 at 4:03 PM, Jeff Janeswrote: > 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
[HACKERS] brin_summarize_new_values error checking
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. Cheers, Jeff [1] http://www.postgresql.org/message-id/CAHGQGwH=m1baejpqdpjjcneqwg8xa+p8sb+zsvhvwh6gl2j...@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