- 15 Apr, 2016 2 commits
-
-
Andres Freund authored
These aren't valid C89. Found thanks to gcc's -Wc90-c99-compat. These exist in differing places in most supported branches.
-
Andres Freund authored
-
- 14 Apr, 2016 10 commits
-
-
Tom Lane authored
When re-reading an update involving both an old tuple and a new tuple from disk, reorderbuffer.c was careless about whether the new tuple is suitably aligned for direct access --- in general, it isn't. We'd missed seeing this in the buildfarm because the contrib/test_decoding tests exercise this code path only a few times, and by chance all of those cases have old tuples with length a multiple of 4, which is usually enough to make the access to the new tuple's t_len safe. For some still-not-entirely-clear reason, however, Debian's sparc build gets a bus error, as reported by Christoph Berg; perhaps it's assuming 8-byte alignment of the pointer? The lack of previous field reports is probably because you need all of these conditions to trigger a crash: an alignment-picky platform (not Intel), a transaction large enough to spill to disk, an update within that xact that changes a primary-key field and has an odd-length old tuple, and of course logical decoding tracing the transaction. Avoid the alignment assumption by using memcpy instead of fetching t_len directly, and add a test case that exposes the crash on picky platforms. Back-patch to 9.4 where the bug was introduced. Discussion: <20160413094117.GC21485@msg.credativ.de>
-
Tom Lane authored
Commit 314cbfc5 redefined the signature of this hook as typedef int (*walrcv_receive_type) (char **buffer, int *wait_fd); But in fact the type of the "wait_fd" variable ought to be pgsocket, which is what WaitLatchOrSocket expects, and which is necessary if we want to be able to assign PGINVALID_SOCKET to it on Windows. So fix that.
-
Tom Lane authored
It was declared as "pid_t", which would be fine except that none of the places that printed it in error messages took any thought for the possibility that it's not equivalent to "int". This leads to warnings on some buildfarm members, and could possibly lead to actually wrong error messages on those platforms. There doesn't seem to be any very good reason not to just make it "int"; it's only ever assigned from MyProcPid, which is int. If we want to cope with PIDs that are wider than int, this is not the place to start. Also, fix the comment, which seems to perhaps be a leftover from a time when the field was only a bool? Per buildfarm. Back-patch to 9.5 which has same issue.
-
Tom Lane authored
Section 7.6 was a tad confusing because it specified what LIMIT NULL does, but neglected to do the same for OFFSET NULL, making this look like perhaps a special case or a wrong restatement of the bit about LIMIT ALL. Wordsmith a bit while at it. Per bug #14084.
-
Tom Lane authored
I (tgl) had copied-and-pasted this from pgwin32_accept(), failing to notice that the third parameter should be "int" not "int *". David Rowley
-
Tom Lane authored
For a long time, opclasscmds.c explained that "we do not create a dependency link to the AM [for an opclass or opfamily], because we don't currently support DROP ACCESS METHOD". Commit 473b9328 invented DROP ACCESS METHOD, but it batted only 1 for 2 on adding the dependency links, and 0 for 2 on updating the comments about the topic. In passing, undo the same commit's entirely inappropriate decision to blow away an existing index as a side-effect of create_am.sql.
-
Fujii Masao authored
Commit cfe96ae2 corrected the name of pg_logical_emit_message() in its index entry. But this typo fix caused duplicated index entry because there was another index entry for the function. Spotted by Tom Lane.
-
Stephen Frost authored
As part of reserving the pg_* namespace for default roles and in line with SET ROLE and other previous efforts, disallow settings the role to a default/reserved role using SET SESSION AUTHORIZATION. These checks and restrictions on what is allowed regarding default / reserved roles are under debate, but it seems prudent to ensure that the existing checks at least cover the intended cases while the debate rages on. On me to clean it up if the consensus decision is to remove these checks.
-
Andres Freund authored
Logical messages, added in 3fe3511d, during decoding failed to filter messages emitted in other databases and messages emitted "under" a replication origin the output plugin isn't interested in. Add tests to verify that both types of filtering actually work. While touching message.sql remove hunk obsoleted by d25379eb. Bump XLOG_PAGE_MAGIC because xl_logical_message changed and because 3fe3511d had omitted doing so. 3fe3511d additionally didn't bump catversion, but 7a542700 has done so since. Author: Petr Jelinek Reported-By: Andres Freund Discussion: 20160406142513.wotqy3ba3kanr423@alap3.anarazel.de
-
Andres Freund authored
The current definition of init_spin_delay (introduced recently in 48354581) wasn't C89 compliant. It's not legal to refer to refer to non-constant expressions, and the ptr argument was one. This, as reported by Tom, lead to a failure on buildfarm animal pademelon. The pointer, especially on system systems with ASLR, isn't super helpful anyway, though. So instead of making init_spin_delay into an inline function, make s_lock_stuck() report the function name in addition to file:line and change init_spin_delay() accordingly. While not a direct replacement, the function name is likely more useful anyway (line numbers are often hard to interpret in third party reports). This also fixes what file/line number is reported for waits via s_lock(). As PG_FUNCNAME_MACRO is now used outside of elog.h, move it to c.h. Reported-By: Tom Lane Discussion: 4369.1460435533@sss.pgh.pa.us
-
- 13 Apr, 2016 6 commits
-
-
Tom Lane authored
As reported by Michael Feld, pg_upgrade'ing an installation having extensions with operator families that contain just a single operator class failed to reproduce the extension membership of those operator families. This caused no immediate ill effects, but would create problems when later trying to do a plain dump and restore, because the seemingly-not-part-of- the-extension operator families would appear separately in the pg_dump output, and then would conflict with the families created by loading the extension. This has been broken ever since extensions were introduced, and many of the standard contrib extensions are affected, so it's a bit astonishing nobody complained before. The cause of the problem is a perhaps-ill-considered decision to omit such operator families from pg_dump's output on the grounds that the CREATE OPERATOR CLASS commands could recreate them, and having explicit CREATE OPERATOR FAMILY commands would impede loading the dump script into pre-8.3 servers. Whatever the merits of that decision when 8.3 was being written, it looks like a poor tradeoff now. We can fix the pg_upgrade problem simply by removing that code, so that the operator families are dumped explicitly (and then will be properly made to be part of their extensions). Although this fixes the behavior of future pg_upgrade runs, it does nothing to clean up existing installations that may have improperly-linked operator families. Given the small number of complaints to date, maybe we don't need to worry about providing an automated solution for that; anyone who needs to clean it up can do so with manual "ALTER EXTENSION ADD OPERATOR FAMILY" commands, or even just ignore the duplicate-opfamily errors they get during a pg_restore. In any case we need this fix. Back-patch to all supported branches. Discussion: <20228.1460575691@sss.pgh.pa.us>
-
Andres Freund authored
The recent patch to make Pin/UnpinBuffer lockfree in the hot path (48354581), accidentally used pg_atomic_fetch_or_u32() in MarkLocalBufferDirty(). Other code operating on local buffers was careful to only use pg_atomic_read/write_u32 which just read/write from memory; to avoid unnecessary overhead. On its own that'd just make MarkLocalBufferDirty() slightly less efficient, but in addition InitLocalBuffers() doesn't call pg_atomic_init_u32() - thus the spinlock fallback for the atomic operations isn't initialized. That in turn caused, as reported by Tom, buildfarm animal gaur to fail. As those errors are actually useful against this type of error, continue to omit - intentionally this time - initialization of the atomic variable. In addition, add an explicit note about only using pg_atomic_read/write on local buffers's state to BufferDesc's description. Reported-By: Tom Lane Discussion: 1881.1460431476@sss.pgh.pa.us
-
Tom Lane authored
It's silly to define these counts as narrower than they might someday need to be. Also, I believe that the BLCKSZ * nflush calculation in mdwriteback was capable of overflowing an int.
-
Tom Lane authored
Commit 428b1d6b introduced the use of msync() for flushing dirty data from the kernel's file buffers. Several portability issues were overlooked, though: * Not all implementations of mmap() think that nbytes == 0 means "map the whole file". To fix, use lseek() to find out the true length. Fix callers of pg_flush_data to be aware that nbytes == 0 may result in trashing the file's seek position. * Not all implementations of mmap() will accept partial-page mmap requests. To fix, round down the length request to whatever sysconf() says the page size is. (I think this is OK from a portability standpoint, because sysconf() is required by SUS v2, and we aren't trying to compile this part on Windows anyway. Buildfarm should let us know if not.) * On 32-bit machines, the file size might exceed the available free address space, or even exceed what will fit in size_t. Check for the latter explicitly to avoid passing a false request size to mmap(). If mmap fails, silently fall through to the next implementation method, rather than bleating to the postmaster log and giving up. * mmap'ing directories fails on some platforms, and even if it works, msync'ing the directory is quite unlikely to help, as for that matter are the other flush implementations. In pre_sync_fname(), just skip flush attempts on directories. In passing, copy-edit the comments a bit. Stas Kelvich and myself
-
Tom Lane authored
Fix misleading syntax summary (there cannot be a space between colH and scolH). Provide a link from the existing crosstab() function's documentation to \crosstabview. Copy-edit the command's description. Christoph Berg and Tom Lane
-
Robert Haas authored
Makes no difference, but it's cleaner this way. Michael Paquier
-
- 12 Apr, 2016 13 commits
-
-
Tom Lane authored
I've seen one too many "could not bind IPv4 socket: No error" log entries from the Windows buildfarm members. Per previous discussion, this is likely caused by the fact that we're doing nothing to translate WSAGetLastError() to errno. Put in a wrapper layer to do that. If this works as expected, it should get back-patched, but let's see what happens in the buildfarm first. Discussion: <4065.1452450340@sss.pgh.pa.us>
-
Robert Haas authored
The original patch kind of ignored the fact that we were doing something different from a costing point of view, but nobody noticed. This patch fixes that oversight. David Rowley
-
Fujii Masao authored
That unused function was introduced as a sample because synchronous replication or replication monitoring tools might need it in the future. Recently commit 989be081 added the function SyncRepGetOldestSyncRecPtr which provides almost the same functionality for multiple synchronous standbys feature. So it's time to remove that unused sample function. This commit does that.
-
Tom Lane authored
Per discussion, this gives potential users of the hook more flexibility, because they can build custom Paths that implement only one stage of upper processing atop core-provided Paths for earlier stages.
-
Tom Lane authored
Coverity complained about this code, not without reason because it was rather messy. Adjust it to not scribble on the passed string; that adds one malloc/free cycle per column name, which is going to be insignificant in context. We can actually const-ify both the string argument and the PGresult. Daniel Verité, with some further cleanup by me
-
Kevin Grittner authored
On a big NUMA machine with 1000 connections in saturation load there was a performance regression due to spinlock contention, for acquiring values which were never used. Just fill with dummy values if we're not going to use them. This patch has not been benchmarked yet on a big NUMA machine, but it seems like a good idea on general principle, and it seemed to prevent an apparent 2.2% regression on a single-socket i7 box running 200 connections at saturation load.
-
Tom Lane authored
Rename this function to GenericXLogRegisterBuffer() to make it clearer what it does, and leave room for other sorts of "register" actions in future. Also, replace its "bool isNew" argument with an integer flags argument, so as to allow adding more flags in future without an API break. Alexander Korotkov, adjusted slightly by me
-
Tom Lane authored
The previous coding could allow the contents of the "hole" between pd_lower and pd_upper to diverge during replay from what it had been when the update was originally applied. This would pose a problem if checksums were in use, and in any case would complicate forensic comparisons between master and slave servers. So force the "hole" to contain zeroes, both at initial application of a generically-logged action, and at replay. Alexander Korotkov, adjusted slightly by me
-
Teodor Sigaev authored
Added to ensure that bloom index pages can be distinguished from other pages by pg_filedump. Because there wasn't any public/production versions before, it doesn't pay attention to any compatibility issues. Per notice from Tom Lane
-
Tom Lane authored
In commit b0e40d18, I should have just removed the /D switch defining WIN64. The reason the code worked before is that all Windows64 compilers automatically predefine _WIN64. Perhaps at one time we had code that depended on WIN64 being defined, but it's long gone, and we should not encourage any reappearance. Per discussion with Christian Ullrich.
-
Stephen Frost authored
It's 2016 these days (no, not entirely sure how we got here either). Pointed out by Amit Langote
-
Peter Eisentraut authored
-
Tom Lane authored
When IF NOT EXISTS was added to CREATE TABLE AS, this logic didn't get the memo, possibly resulting in an Assert failure. It looks like there would have been no ill effects in a non-Assert build, though. Back-patch to 9.5 where the IF NOT EXISTS option was added. Stas Kelvich
-
- 11 Apr, 2016 9 commits
-
-
Tom Lane authored
Everyplace else thinks it's _WIN64, so make these places fall in line. The pg_regress.c usage is not going to result in any change in behavior, only suppressing (or not) a compiler warning about downcasting HANDLEs. So there seems no need for back-patching there. The libpq/win32.mak usage might represent an actual bug, if anyone were using this script to build for Windows64, which perhaps nobody is. Given the lack of field complaints, no back-patch here either. pg_regress.c problem found by Christian Ullrich, the other by me.
-
Tom Lane authored
It turns out that those PyErr_Clear() calls I removed from plpy_elog.c in 7e3bb080 et al were not quite as random as they appeared: they mask a Python 2.3.x bug. (Specifically, it turns out that PyType_Ready() can fail if the error indicator is set on entry, and PLy_traceback's fetch of frame.f_code may be the first operation in a session that requires the "frame" type to be readied. Ick.) Put back the clear call, but in a more centralized place closer to what it's protecting, and this time with a comment warning what it's really for. Per buildfarm member prairiedog. Although prairiedog was only failing on HEAD, it seems clearly possible for this to occur in older branches as well, so back-patch to 9.2 the same as the previous patch.
-
Kevin Grittner authored
I was initially concerned that the some of the hundreds of references to BufferGetPage() where the literal BGP_NO_SNAPSHOT_TEST were passed might not optimize as well as a macro, leading to some hard-to-find performance regressions in corner cases. Inspection of disassembled code has shown identical code at all inspected locations, and the size difference doesn't amount to even one byte per such call. So make it readable. Per gripes from Álvaro Herrera and Tom Lane
-
Kevin Grittner authored
It was incorrectly declared as a volatile pointer to a non-volatile structure. Eliminate the OldSnapshotControl struct definition; it is really not needed. Pointed out by Tom Lane. While at it, add OldSnapshotControlData to pgindent's list of structures.
-
Peter Eisentraut authored
-
Stephen Frost authored
To avoid any possible overlap with existing roles on a system when doing a 'make installcheck', use role names which start with 'regress_'. Pointed out by Tom.
-
Peter Eisentraut authored
-
Tom Lane authored
Commit 5c3c3cd0 plastered "volatile" on a bunch of variables in PLy_output(), but removed the one that actually mattered, ie the one on "oldcontext". This allows some versions of clang to generate code in which "oldcontext" has been trashed when control reaches the PG_CATCH block. Per buildfarm member tick.
-
Peter Eisentraut authored
Some things in src/include/fe_utils require libpq headers, so add libpq's include path to the command line used here.
-