Re: [HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
Hello, At Tue, 14 Apr 2015 12:10:35 +0900, Etsuro Fujita wrote in <552c852b.2050...@lab.ntt.co.jp> > On 2015/04/13 23:25, Jim Nasby wrote: > > On 4/13/15 4:58 AM, Etsuro Fujita wrote: > >> On 2015/04/10 21:40, Etsuro Fujita wrote: > >>> On 2015/04/09 12:07, Etsuro Fujita wrote: > I'll update the patch, which will contain only an infrastructure for > this in the PG core, and will not contain any postgres_fdw change. > >>> > >>> I updated the patch based on your comments. Updated patch attached. In > >>> the patch the following FDW APIs have been proposed: > >>> > >>> + RowMarkType > >>> + GetForeignRowMarkType (LockClauseStrength strength); > >>> > >>> + bool > >>> + LockForeignRow (EState *estate, > >>> + ExecRowMark *erm, > >>> + ItemPointer tupleid); > >>> > >>> + HeapTuple > >>> + FetchForeignRow (EState *estate, > >>> + ExecRowMark *erm, > >>> + ItemPointer tupleid); > >>> > >>> I think that these APIs allow the FDW that has TIDs to use the rowmark > >>> options such as ROW_MARK_REFERENCE, ROW_MARK_SHARE and > >>> ROW_MARK_EXCLUSIVE for its foreign tables so as to match the local > >>> semantics exactly, for example. > >>> > >>> As you mentioned, it would be better to add helper functions to see > >>> whether the foreign table is referenced by any ExecRowMarks. ISTM that > >>> an easy way to do that is to modify ExecFindRowMark() so that it allows > >>> for the missing case. I didn't contain such functions in the patch, > >>> though. > >> > >> I added that function and modified docs a bit. Please find attached an > >> updated version of the patch. > > > > Why aren't we allowing SELECT FOR KEY SHARE? > > I omitted that mode (and the FOR NO KEY UPDATE mode) for simplicity, but > both modes have been allowed. However, I'm not sure if those modes are > useful because we don't have information about keys for a remote table. If I understand this correctly, the two lock modes are no relation with key column definitions, and are provided as a weaker lock in exchange for some risks. Like advisory locks. we can FOR_NO_KEY_UPDATE in the trunsactions that evetually update "key columns" but also should accept the unexpected result. In other words, the "key" in the context of "FOR NO KEY UPDATE" and "FOR KEY SHARE" is only in the programmers' minds. Apart from feasibility, I don't see no resason to omit them if this is correct. As an example, the following operations cause an "unexpected" result. Ex. 1 Session A=# create table t (a int primary key, b int); Session A=# insert into t (select a, 1 from generate_series(0, 99) a); Session A=# begin; Session A=# select * from t where a = 1 for no key update; Session B=# begin; Session B=# select * from t where a = 1 for key share; Session B=# update t set a = -a where a = 1; Ex. 2 Session A=# create table t (a int, b int); -- a is the key in mind. Session A=# insert into t (select a, 1 from generate_series(0, 99) a); Session A=# begin; Session A=# select * from t where a = 1 for no key update; Session B=# begin; Session B=# select * from t where a = 1 for key share; Session A=# update t set a = -a where a = 1; UPDATE 1 Session A=# commit; Session B=# select * from t where a = 1; (0 rows) -- Woops. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
Hanada-san, > Thanks for further review, but I found two bugs in v10 patch. > I’ve fixed them and wrapped up v11 patch here. > > * Fix bug about illegal column order > > Scan against a base relation returns columns in order of column definition, > but > its target list might require different order. This can be resolved by tuple > projection in usual cases, but pushing down joins skips the step, so we need > to > treat it in remote query. > > Before this fix, deparseProjectionSql() was called only for queries which have > ctid or whole-row reference in its target list, but it was a too-much > optimization. > We always need to call it, because usual column list might require ordering > conversion. Checking ordering is not impossible, but it seems useless effort. > > Another way to resolve this issue is to reorder SELECT clause of a query for > base > relation if it was a source of a join, but it requires stepping back in > planning, > so the fix above was chosen. > > "three tables join" test case is also changed to check this behavior. > Sorry for my oversight. Yep, var-node reference on join relation cannot expect any column orders predefined like as base relations. All reasonable way I know is, relying on the targetlist of the RelOptInfo that contains all the referenced columns in the later stage. > * Fix bug of duplicate fdw_ps_tlist contents. > > I coded that deparseSelectSql passes fdw_ps_tlist to deparseSelectSql for > underlying RelOptInfo, but it causes redundant entries in fdw_ps_tlist in > cases > of joining more than two foreign tables. I changed to pass NULL as > fdw_ps_tlist > for recursive call of deparseSelectSql. > It's reasonable, and also makes performance benefit because descriptor constructed based on the ps_tlist will match expected result tuple, so it allows to avoid unnecessary projection. > * Fix typos > > Please review the v11 patch, and mark it as “ready for committer” if it’s ok. > It's OK for me, and wants to be reviewed by other people to get it committed. > In addition to essential features, I tried to implement relation listing in > EXPLAIN > output. > > Attached explain_forein_join.patch adds capability to show join combination of > a ForeignScan in EXPLAIN output as an additional item “Relations”. I thought > that using array to list relations is a good way too, but I chose one string > value > because users would like to know order and type of joins too. > A bit different from my expectation... I expected to display name of the local foreign tables (and its alias), not remote one, because all the local join logic displays local foreign tables name. Is it easy to adjust isn't it? Probably, all you need to do is, putting a local relation name on the text buffer (at deparseSelectSql) instead of the deparsed remote relation. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei -- 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] Additional role attributes && superuser review
Robert, * Stephen Frost (sfr...@snowman.net) wrote: > * Robert Haas (robertmh...@gmail.com) wrote: > > On Thu, Apr 2, 2015 at 12:53 AM, Stephen Frost wrote: > > > Clearly, further testing and documentation is required and I'll be > > > getting to that over the next couple of days, but it's pretty darn late > > > and I'm currently getting libpq undefined reference errors, probably > > > because I need to set LD_LIBRARY_PATH, but I'm just too tired to now. :) > > > > > > Thoughts? > > > > The tricky part of this seems to me to be the pg_dump changes. The > > new catalog flag seems a little sketchy to me; wouldn't it be better > > to make the dump flag into an enum, DUMP_EVERYTHING, DUMP_ACL, > > DUMP_NONE? > > I agree that the pg_dump changes are a very big part of this change. > I'll look at using an enum and see if that would work. I've implemented this approach and there are things which I like about it and things which I don't. I'd love to hear your thoughts. As mentioned previously, this patch does not break the pg_stat_activity or pg_stat_replication view APIs. Similairly, the functions which directly feed those views return the same results they did previously, there are just additional functions now which provide everything, and view on top of those, for users who are GRANT'd access to them. This does change the API of a few functions which previously allowed roles with the "replication" attribute to call them. We could provide additional functions to provide both paths but I don't believe it's really necessary. Indeed, I feel it's better for administrators to explicit grant access to those functions instead. Note that this doesn't use an enum but a bit field for which components of a given object should be dumped out. While I like that in general, it meant a lot of changes and I'll be the first to admit that I've not tested every possible pg_dump option permutation to make sure that the correct results are returned. I plan to do that in the coming weeks and will address any issues found. Is this, more-or-less, what you were thinking of? I looked at removing the relatively grotty options (dataOnly, aclsSkip, etc) and it didn't appear trivial to use the bitmask instead. I can look into that further though, as I do feel that it'd be good if we could reduce our dependence on those options in favor of the bitmask approach. Thoughts? Thanks! Stephen From dd682d4d9dc7f25ae38bb5fc5ed5082c5071 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Thu, 19 Mar 2015 14:49:26 -0400 Subject: [PATCH] Use GRANT for access to privileged functions Historically, we have hard-coded the permissions for privileged functions in C code (eg: if (superuser()) then allow X, else don't). Unfortunately, that's pretty limiting and means that functions which are useful for monitoring require that the user calling them be a superuser. That's a problem because, generally speaking, monitoring systems should not have more access than they need and certainly should not have write access to a system. Thankfully, we have a very handy and complex privilege system for managing who has access to what already built into PG. This is the GRANT system which has existed since very near the beginning of PG. Therefore, provide a set of system functions which are not able to be executed by all users by default and allow administrators to grant access to those functions for the users (eg: monitoring or other roles) where they feel it is appropriate. We avoid breaking existing APIs for the system views by providing backwards compatible functions which continue to filter in the C code based on the user's credentials, where that is possible. For a few functions (eg: pg_logical_slot_* and friends), it makes more sense to break compatibility as they are relatively new and require admins to GRANT access to those functions explicitly on upgrade (this should be noted in the release notes). Other functions, which allowed access based on role attribute 'replication', may need to have GRANTs applied to them, but note that replication connections do not go through the normal GRANT system and therefore tools such as pg_basebackup will not be impacted by this change. In general, this change requires administrators to be more explicit about which roles have access to these functions. Last, but certainly not least, this changes pg_dump to include ACLs for the pg_catalog schema. This is necessary as administrators are now expected and encouraged to set privileges on functions and perhaps views differently from their defaults and we need to preserve those settings. --- contrib/test_decoding/expected/permissions.out | 17 +- contrib/test_decoding/sql/permissions.sql |9 + src/backend/access/transam/xlogfuncs.c | 30 - src/backend/catalog/system_views.sql | 72 ++ src/backend/replication/logical/logicalfuncs.c | 11 - src/backend/replication/slotfuncs.c| 15 - src
Re: [HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
On 2015/04/13 23:25, Jim Nasby wrote: > On 4/13/15 4:58 AM, Etsuro Fujita wrote: >> On 2015/04/10 21:40, Etsuro Fujita wrote: >>> On 2015/04/09 12:07, Etsuro Fujita wrote: I'll update the patch, which will contain only an infrastructure for this in the PG core, and will not contain any postgres_fdw change. >>> >>> I updated the patch based on your comments. Updated patch attached. In >>> the patch the following FDW APIs have been proposed: >>> >>> + RowMarkType >>> + GetForeignRowMarkType (LockClauseStrength strength); >>> >>> + bool >>> + LockForeignRow (EState *estate, >>> + ExecRowMark *erm, >>> + ItemPointer tupleid); >>> >>> + HeapTuple >>> + FetchForeignRow (EState *estate, >>> + ExecRowMark *erm, >>> + ItemPointer tupleid); >>> >>> I think that these APIs allow the FDW that has TIDs to use the rowmark >>> options such as ROW_MARK_REFERENCE, ROW_MARK_SHARE and >>> ROW_MARK_EXCLUSIVE for its foreign tables so as to match the local >>> semantics exactly, for example. >>> >>> As you mentioned, it would be better to add helper functions to see >>> whether the foreign table is referenced by any ExecRowMarks. ISTM that >>> an easy way to do that is to modify ExecFindRowMark() so that it allows >>> for the missing case. I didn't contain such functions in the patch, though. >> >> I added that function and modified docs a bit. Please find attached an >> updated version of the patch. > > Why aren't we allowing SELECT FOR KEY SHARE? I omitted that mode (and the FOR NO KEY UPDATE mode) for simplicity, but both modes have been allowed. However, I'm not sure if those modes are useful because we don't have information about keys for a remote table. Best regards, Etsuro Fujita -- 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] FPW compression leaks information
On Sun, Apr 12, 2015 at 8:38 PM, Heikki Linnakangas wrote: > Care to name some? This is certainly quite cumbersome to exploit, but it's > doable. > > We've talked a lot about covert channels and timing attacks on RLS, but this > makes me more worried because you can attack passwords stored in pg_authid. Well, one thing to think about is that, if we're going to take this kind of attack seriously, it could be used on user data, too. I mean, you've just got to be able to get a row into the same block as the row you want to find out something about, and there's no reason that need be true only for pg_authid. And, even if you ban access to information on the pg_xlog insert location, what's to prevent someone from gleaning similar information via a timing attack on the compression itself? You can even get some information from figuring out, by trial and error, how much you need to expand a row to get it to spill over into another block. If there happens to be enough free-space on the page where the row is located to allow a HOT update, you can repeatedly update it until it gets moved to some unrelated page. Granted, knowing the length of an unseen row isn't as good as knowing the contents, but it's still potentially useful information. As to RLS - yeah, that's where I think a lot of the possible covert channel attacks are. But it doesn't have to be RLS per se. It can be, for example, a table some of whose contents you expose via a security barrier view. It can be a security-definer function that you call and it returns some data that you don't have rights to view directly. Basically, it can be any table to which you have some access, but not complete access. Then you can use timing attacks, scrutinize EXPLAIN plans for clues, look at CTIDs, and so on. Basically, I'm worried that it's going to be completely impractical to prevent a certain amount of information leakage when you have partial access to a table, and that in a largely-futile effort to try to prevent it, we'll end up making a whole bunch of things (like the WAL insert position) super-user only, and that this will in fact be a net reduction in security because it'll encourage people to use the superuser account more. Perhaps that concern is ill-founded, but that's what I'm worried about. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] pg_rewind tests
On Tue, Apr 14, 2015 at 12:32 AM, Heikki Linnakangas wrote: > Applied, thanks! Thanks. Now, for Windows... -- Michael -- 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] pg_rewind tests
On 04/02/2015 04:56 AM, Michael Paquier wrote: On Tue, Mar 31, 2015 at 4:37 PM, Michael Paquier wrote: While looking at that I noticed two additional issues: - In remote mode, the connection string to the promoted standby was incorrect when running pg_rewind, leading to connection errors - At least in my environment, a sleep of 1 after the standby promotion was not sufficient to make the tests work. While working on another patch for TAP tests, I noticed that relying on environment variables to run tests is a bad idea as well, as other tests do not do it, so instead is a patch that refactors the tests so as they do not use environment variables and so as it is not necessary to pass arguments to prove. The trick is to use sub-routines in each test, and invoke this subroutine for both 'local' and 'remote'. This changes the order the tests are run, but I guess that's not a big deal as long as the tests are run, and this approach looks more solid to me as it makes pg_rewind's tests more consistent with the rest. The log files of each test are still separated the same way as before. Applied, thanks! - Heikki -- 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] pg_rewind tests
On 04/10/2015 04:01 PM, Alvaro Herrera wrote: Do these tests work in VPATH builds? I now get a failure in "make check-world" (both with and without Michael's patch). "make check" in src/bin/pg_rewind dies with the output below. If I change "./pg_rewind" to "pg_rewind", tests pass, but I wonder if the full path to the temp-install bindir should be used instead. Just "pg_rewind" seems correct, that's what all the other TAP tests do. The Makefile target that calls the perl script sets PATH to point to the temporary installation's bin dir. Fixed. - Heikki -- 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] Bogus WAL segments archived after promotion
On 04/01/2015 07:12 PM, Bruce Momjian wrote: On Fri, Dec 19, 2014 at 10:26:34PM +0200, Heikki Linnakangas wrote: On 12/19/2014 02:55 PM, Heikki Linnakangas wrote: I'm thinking that we should add a step to promotion, where we scan pg_xlog for any segments higher than the timeline switch point, and remove them, or mark them with .done so that they are not archived. There might be some real WAL that was streamed from the primary, but not yet applied, but such WAL is of no interest to that server anyway, after it's been promoted. It's a bit disconcerting to zap WAL that's valid, even if doesn't belong to the current server's timeline history, because as a general rule it's good to avoid destroying evidence that might be useful in debugging. There isn't much difference between removing them immediately and marking them as .done, though, because they will eventually be removed/recycled anyway if they're marked as .done. This is what I came up with. This patch removes the suspect segments at timeline switch. The alternative of creating .done files for them would preserve more evidence for debugging, but OTOH it would also be very confusing to have valid-looking WAL segments in pg_xlog, with .done files, that in fact contain garbage. The patch is a bit longer than it otherwise would be, because I moved the code to remove a single file from RemoveOldXlogFiles() to a new function. I think that makes it more readable in any case, simply because it was so deeply indented in RemoveOldXlogFiles. Where are we on this? I didn't hear any better ideas, so committed this now. - Heikki -- 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] EvalPlanQual behaves oddly for FDW queries involving system columns
On 4/13/15 4:58 AM, Etsuro Fujita wrote: > On 2015/04/10 21:40, Etsuro Fujita wrote: >> On 2015/04/09 12:07, Etsuro Fujita wrote: >>> I'll update the patch, which will contain only an infrastructure for >>> this in the PG core, and will not contain any postgres_fdw change. >> >> I updated the patch based on your comments. Updated patch attached. In >> the patch the following FDW APIs have been proposed: >> >> + RowMarkType >> + GetForeignRowMarkType (LockClauseStrength strength); >> >> + bool >> + LockForeignRow (EState *estate, >> + ExecRowMark *erm, >> + ItemPointer tupleid); >> >> + HeapTuple >> + FetchForeignRow (EState *estate, >> + ExecRowMark *erm, >> + ItemPointer tupleid); >> >> I think that these APIs allow the FDW that has TIDs to use the rowmark >> options such as ROW_MARK_REFERENCE, ROW_MARK_SHARE and >> ROW_MARK_EXCLUSIVE for its foreign tables so as to match the local >> semantics exactly, for example. >> >> As you mentioned, it would be better to add helper functions to see >> whether the foreign table is referenced by any ExecRowMarks. ISTM that >> an easy way to do that is to modify ExecFindRowMark() so that it allows >> for the missing case. I didn't contain such functions in the patch, though. > > I added that function and modified docs a bit. Please find attached an > updated version of the patch. Why aren't we allowing SELECT FOR KEY SHARE? -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- 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] Revisiting Re: BUG #8532: postgres fails to start with timezone-data >=2013e
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 11/04/15 05:30 PM, Tom Lane wrote: > Ian Stakenvicius writes: >> Hey all -- so I know that Gentoo Linux is likely the only >> platform this bug occurs under, but i got annoyed enough with it >> that I decided to write a patch to fix this issue once and for >> all (or at least, help keep it from happening). > >> That thread in question actually dealt with crashing on startup >> in postgresql-9.1 and earlier, but all versions including the >> latest still suffer from the inability to load timezone data via >> the pg_timezone_* tables if /usr/share/zoneinfo contains >> filesystem loops. > >> To that end, the following helps resolve this issue by ensuring >> single-level filesystem loops are detected and skipped. The top >> half of the patch only applies to postgresql-9.1 and earlier, >> while the second half applies to all versions. > > This patch doesn't look right at all. In the first place, tzdirsub > is the entire subpath, not necessarily just one filename. In the > second place, limiting the strncmp comparison to > strlen(direntry->d_name) exposes you to false matches --- for > example, a current d_name of "foo" would be thought to match a > tzdirsub path of "foobar". In the third place, relying on > basename(1) does not seem advisable for portability reasons --- > use something from src/port/path.c instead. > That would be due to a misinterpretation of tzdirsub on my part, then ; inspecting the code it had seemed to me that for each level of recursion, tzdirsub is incremented so that it only contains the target working directory rather than the entire subpath. Yes, false matches are also a possibility when the directory being tested is a substring of the parent directory, however based on the format of zoneinfo it seems unlikely this will occur -- the closest example we have is "America/Indiana/Indianapolis" , except that to trigger the false positive it would have to be structured as "America/Indianapolis/Indiana"; it was a risk and certainly could be further mitigated via strcmp() instead or some other check. Basename is posix (and i used it in the manner necessary for the posix implementation rather than the gnu implementation), so it should be available on most platforms, but certainly a different implementation could be used to obtain the top directory instead of stripping it from the path via basename. > It would probably be a good idea to be more specific about what > sorts of loops you are hoping to catch, because this surely won't > detect all possible loops as-is. My original goal was to simply stop recursion when the 'posix' subdir of /usr/share/zoneinfo is a symlink to the current directory (ie: 'posix' -> '.') -- the first iteration should still be processed (ie, posix and all subdirs), but posix/posix would not be processed. I wanted to do it generically though so that it could prevent such recursion on at least a subset of possible filesystem loops. I know that this code doesn't detect multi-level filesystem loops (ie: 'posix/something' -> '..') and that the code necessary to detect that would need to be much more complicated. Also I felt that if any filesystem loop were to exist within /usr/share/zoneinfo, likely it would be a single-level loop such as the 'posix' -> '.' one I'm dealing with on Gentoo. Thanks for the feedback; I'll look into adjusting the patch and post back a better version in the future. -BEGIN PGP SIGNATURE- Version: GnuPG v2 iF4EAREIAAYFAlUrzHAACgkQ2ugaI38ACPD78AD/bxjL6YSrOVxwlufLF0BeJmXT wrkPUxUdG0i6UA8GoiABALGiepu/ZLhdv/80pOapmBOZmaCygChX9dIsgXpqtIuE =5/QU -END PGP SIGNATURE- -- 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] FPW compression leaks information
* Heikki Linnakangas (hlinn...@iki.fi) wrote: > On 04/10/2015 05:17 AM, Robert Haas wrote: > >On Apr 9, 2015, at 8:51 PM, Heikki Linnakangas wrote: > >>What should we do about this? > > > >I bet that there are at least 1000 covert channel attacks that are more > >practically exploitable than this. > > Care to name some? This is certainly quite cumbersome to exploit, > but it's doable. I don't see any good reason to expose this information to every user on the system, regardless of how easy (or not easy) it is to exploit. There's a bunch of information which we want monitoring systems to be able to gather but which shouldn't be generally available and this is just another example of that. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] "rejected" vs "returned with feedback" in new CF app
David G. Johnston wrote: > Can we create a "fake" CF time period into which all of these "waiting on > author" entries can be placed and readily browsed/found instead of leaving > them in whatever CF they happened to stall in? This seems a good idea to me -- not a "fake CF", but a page listing all the Returned With Feedback patches, regardless of which commitfest(s) they're linked onto. -- Álvaro Herrerahttp://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] pg_rewind in contrib
On 03/26/2015 09:45 PM, Arthur Silva wrote: On Mar 26, 2015 4:20 AM, "Vladimir Borodin" wrote: 26 марта 2015 г., в 7:32, Michael Paquier написал(а): On Thu, Mar 26, 2015 at 12:23 PM, Venkata Balaji N wrote: If the master is crashed or killed abruptly, it may not be possible to do a rewind. Is my understanding correct ? Yep. This is mentioned in the documentation: http://www.postgresql.org/docs/devel/static/app-pgrewind.html "The target server must shut down cleanly before running pg_rewind>>. You can start old master, wait for crash recovery to complete, stop it cleanly and then use pg_rewind. It works. Shouldn't we have a flag so it does that automatically if necessary? Might be handy, but currently pg_rewind never invokes postgres or other binaries, so it would be a fair amount of new functionality to implement that. Let's keep it simple for now. - Heikki -- 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] EvalPlanQual behaves oddly for FDW queries involving system columns
On 2015/04/10 21:40, Etsuro Fujita wrote: > On 2015/04/09 12:07, Etsuro Fujita wrote: >> I'll update the patch, which will contain only an infrastructure for >> this in the PG core, and will not contain any postgres_fdw change. > > I updated the patch based on your comments. Updated patch attached. In > the patch the following FDW APIs have been proposed: > > + RowMarkType > + GetForeignRowMarkType (LockClauseStrength strength); > > + bool > + LockForeignRow (EState *estate, > + ExecRowMark *erm, > + ItemPointer tupleid); > > + HeapTuple > + FetchForeignRow (EState *estate, > + ExecRowMark *erm, > + ItemPointer tupleid); > > I think that these APIs allow the FDW that has TIDs to use the rowmark > options such as ROW_MARK_REFERENCE, ROW_MARK_SHARE and > ROW_MARK_EXCLUSIVE for its foreign tables so as to match the local > semantics exactly, for example. > > As you mentioned, it would be better to add helper functions to see > whether the foreign table is referenced by any ExecRowMarks. ISTM that > an easy way to do that is to modify ExecFindRowMark() so that it allows > for the missing case. I didn't contain such functions in the patch, though. I added that function and modified docs a bit. Please find attached an updated version of the patch. Best regards, Etsuro Fujita *** a/doc/src/sgml/fdwhandler.sgml --- b/doc/src/sgml/fdwhandler.sgml *** *** 211,216 BeginForeignScan (ForeignScanState *node, --- 211,230 + If the FDW supports SELECT FOR UPDATE/SHARE row locking, + this routine should perform any initialization needed prior to the + actual row locking if the foreign table is referenced in a + SELECT FOR UPDATE/SHARE. To decide whether the foreign + table is referenced in a SELECT FOR UPDATE/SHARE or not, + it's recommended to use ExecFindRowMark with the missing_ok + argument allowing a NULL return if the structure is not. Information + about the row locking is accessible through its return value + (ExecRowMark struct). (The fdw_state + field of ExecRowMark is available for the FDW to store + any private state it needs for the operation.) + + + Note that when (eflags & EXEC_FLAG_EXPLAIN_ONLY) is true, this function should not perform any externally-visible actions; it should only do the minimum required to make the node state valid *** *** 598,603 IsForeignRelUpdatable (Relation rel); --- 612,699 + + FDW Routines For SELECT FOR UPDATE/SHARE row locking + + + + If an FDW supports SELECT FOR UPDATE/SHARE row locking, + it should provide the following callback functions: + + + + + RowMarkType + GetForeignRowMarkType (LockClauseStrength strength); + + + Report which row-marking option to support for a lock strength + associated with a SELECT FOR UPDATE/SHARE request. + This is called at the beginning of query planning. + strength is a member of the LockClauseStrength + enum type. + The return value should be a member of the RowMarkType + enum type. See + src/include/nodes/lockoptions.h and + src/include/nodes/plannodes.h for information about + these enum types. + + + + If the GetForeignRowMarkType pointer is set to + NULL, the default option is selected for any lock strength, + and both LockForeignRow and FetchForeignRow + described below will not be called at query execution time. + + + + + bool + LockForeignRow (EState *estate, + ExecRowMark *erm, + ItemPointer tupleid); + + + Lock one tuple in the foreign table. + estate is global execution state for the query. + erm is the ExecRowMark struct describing + the target foreign table. + tupleid identifies the tuple to be locked. + This function should return true, if the FDW lock the tuple + successfully. Otherwise, return false. + + + + If the LockForeignRow pointer is set to + NULL, attempts to lock rows will fail + with an error message. + + + + + HeapTuple + FetchForeignRow (EState *estate, + ExecRowMark *erm, + ItemPointer tupleid); + + + Fetch one tuple from the foreign table. + estate is global execution state for the query. + erm is the ExecRowMark struct describing + the target foreign table. + tupleid identifies the tuple to be fetched. + This function should return the fetched tuple, if the FDW fetch the + tuple successfully. Otherwise, return NULL. + + + + If the FetchForeignRow pointer is set to + NULL, attempts to lock rows will fail + with an error message. + + + + FDW Routines for EXPLAIN *** *** 1011,1017 GetForeignServerByNa
