1. 03 Oct, 2018 1 commit
  2. 02 Oct, 2018 7 commits
    • Andrew Dunstan's avatar
      Don't build static libraries on Cygwin · a33245a8
      Andrew Dunstan authored
      Cygwin has been building and linking against static libraries. Although
      a bug this has been relatively harmless until now, when this has caused
      errors due to changes in the way we build certain libraries. So this
      patch makes things work the way we always intended, namely that we would
      link against the dynamic libraries (cygpq.dll etc.) and just not build
      the static libraries. The downstream packagers have been doing this for
      some time, so this just aligns with their practice.
      
      Extracted from a patch by Marco Atzeri, with a suggestion from Tom Lane.
      
      Discussion: https://postgr.es/m/1056.1538235347@sss.pgh.pa.us
      a33245a8
    • Tom Lane's avatar
      Change rewriter/planner/executor/plancache to depend on RTE rellockmode. · 6e35939f
      Tom Lane authored
      Instead of recomputing the required lock levels in all these places,
      just use what commit fdba460a made the parser store in the RTE fields.
      This already simplifies the code measurably in these places, and
      follow-on changes will remove a bunch of no-longer-needed infrastructure.
      
      In a few cases, this change causes us to acquire a higher lock level
      than we did before.  This is OK primarily because said higher lock level
      should've been acquired already at query parse time; thus, we're saving
      a useless extra trip through the shared lock manager to acquire a lesser
      lock alongside the original lock.  The only known exception to this is
      that re-execution of a previously planned SELECT FOR UPDATE/SHARE query,
      for a table that uses ROW_MARK_REFERENCE or ROW_MARK_COPY methods, might
      have gotten only AccessShareLock before.  Now it will get RowShareLock
      like the first execution did, which seems fine.
      
      While there's more to do, push it in this state anyway, to let the
      buildfarm help verify that nothing bad happened.
      
      Amit Langote, reviewed by David Rowley and Jesper Pedersen,
      and whacked around a bit more by me
      
      Discussion: https://postgr.es/m/468c85d9-540e-66a2-1dde-fec2b741e688@lab.ntt.co.jp
      6e35939f
    • Andres Freund's avatar
      Use slots more widely in tuple mapping code and make naming more consistent. · cc2905e9
      Andres Freund authored
      It's inefficient to use a single slot for mapping between tuple
      descriptors for multiple tuples, as previously done when using
      ConvertPartitionTupleSlot(), as that means the slot's tuple descriptors
      change for every tuple.
      
      Previously we also, via ConvertPartitionTupleSlot(), built new tuples
      after the mapping even in cases where we, immediately afterwards,
      access individual columns again.
      
      Refactor the code so one slot, on demand, is used for each
      partition. That avoids having to change the descriptor (and allows to
      use the more efficient "fixed" tuple slots). Then use slot->slot
      mapping, to avoid unnecessarily forming a tuple.
      
      As the naming between the tuple and slot mapping functions wasn't
      consistent, rename them to execute_attr_map_{tuple,slot}.  It's likely
      that we'll also rename convert_tuples_by_* to denote that these
      functions "only" build a map, but that's left for later.
      
      Author: Amit Khandekar and Amit Langote, editorialized by me
      Reviewed-By: Amit Langote, Amit Khandekar, Andres Freund
      Discussion:
          https://postgr.es/m/CAJ3gD9fR0wRNeAE8VqffNTyONS_UfFPRpqxhnD9Q42vZB+Jvpg@mail.gmail.com
          https://postgr.es/m/e4f9d743-cd4b-efb0-7574-da21d86a7f36%40lab.ntt.co.jp
      Backpatch: -
      cc2905e9
    • Tom Lane's avatar
      Set snprintf.c's maximum number of NL arguments to be 31. · 625b38ea
      Tom Lane authored
      Previously, we used the platform's NL_ARGMAX if any, otherwise 16.
      The trouble with this is that the platform value is hugely variable,
      ranging from the POSIX-minimum 9 to as much as 64K on recent FreeBSD.
      Values of more than a dozen or two have no practical use and slow down
      the initialization of the argtypes array.  Worse, they cause snprintf.c
      to consume far more stack space than was the design intention, possibly
      resulting in stack-overflow crashes.
      
      Standardize on 31, which is comfortably more than we need (it looks like
      no existing translatable message has more than about 10 parameters).
      I chose that, not 32, to make the array sizes powers of 2, for some
      possible small gain in speed of the memset.
      
      The lack of reported crashes suggests that the set of platforms we
      use snprintf.c on (in released branches) may have no overlap with
      the set where NL_ARGMAX has unreasonably large values.  But that's
      not entirely clear, so back-patch to all supported branches.
      
      Per report from Mateusz Guzik (via Thomas Munro).
      
      Discussion: https://postgr.es/m/CAEepm=3VF=PUp2f8gU8fgZB22yPE_KBS0+e1AHAtQ=09schTHg@mail.gmail.com
      625b38ea
    • Tom Lane's avatar
      Fix corner-case failures in has_foo_privilege() family of functions. · 3d0f68dd
      Tom Lane authored
      The variants of these functions that take numeric inputs (OIDs or
      column numbers) are supposed to return NULL rather than failing
      on bad input; this rule reduces problems with snapshot skew when
      queries apply the functions to all rows of a catalog.
      
      has_column_privilege() had careless handling of the case where the
      table OID didn't exist.  You might get something like this:
      	select has_column_privilege(9999,'nosuchcol','select');
      	ERROR:  column "nosuchcol" of relation "(null)" does not exist
      or you might get a crash, depending on the platform's printf's response
      to a null string pointer.
      
      In addition, while applying the column-number variant to a dropped
      column returned NULL as desired, applying the column-name variant
      did not:
      	select has_column_privilege('mytable','........pg.dropped.2........','select');
      	ERROR:  column "........pg.dropped.2........" of relation "mytable" does not exist
      It seems better to make this case return NULL as well.
      
      Also, the OID-accepting variants of has_foreign_data_wrapper_privilege,
      has_server_privilege, and has_tablespace_privilege didn't follow the
      principle of returning NULL for nonexistent OIDs.  Superusers got TRUE,
      everybody else got an error.
      
      Per investigation of Jaime Casanova's report of a new crash in HEAD.
      These behaviors have been like this for a long time, so back-patch to
      all supported branches.
      
      Patch by me; thanks to Stephen Frost for discussion and review
      
      Discussion: https://postgr.es/m/CAJGNTeP=-6Gyqq5TN9OvYEydi7Fv1oGyYj650LGTnW44oAzYCg@mail.gmail.com
      3d0f68dd
    • Michael Paquier's avatar
      Fix documentation of pgrowlocks using "lock_type" instead of "modes" · 80810ca6
      Michael Paquier authored
      The example used in the documentation is outdated as well.  This is an
      oversight from 0ac5ad51, which bumped up pgrowlocks but forgot some bits
      of the documentation.
      
      Reported-by: Chris Wilson
      Discussion: https://postgr.es/m/153838692816.2950.12001142346234155699@wrigleys.postgresql.org
      Backpatch-through: 9.3
      80810ca6
    • Amit Kapila's avatar
      Test passing expanded-value representations to workers. · 0fd6a8a7
      Amit Kapila authored
      Currently, we don't have an explicit test to pass expanded-value
      representations to workers, so we don't know whether it works on all kind
      of platforms.  We suspect that the current code won't work on
      alignment-sensitive hardware.  This commit will test that aspect and can
      lead to failure on some of the buildfarm machines which we will fix in the
      later commit.
      
      Author: Tom Lane and Amit Kapila
      Discussion: https://postgr.es/m/11629.1536550032@sss.pgh.pa.us
      0fd6a8a7
  3. 01 Oct, 2018 6 commits
    • Michael Paquier's avatar
      Refactor relation opening for VACUUM and ANALYZE · e3a25ab9
      Michael Paquier authored
      VACUUM and ANALYZE share similar logic when it comes to opening a
      relation to work on in terms of how the relation is opened, in which
      order locks are tried and how logs should be generated when something
      does not work as expected.
      
      This commit refactors things so as both use the same code path to handle
      the way a relation is opened, so as the integration of new options
      becomes easier.
      
      Author: Michael Paquier
      Reviewed-by: Nathan Bossart
      Discussion: https://postgr.es/m/20180927075152.GT1659@paquier.xyz
      e3a25ab9
    • Peter Eisentraut's avatar
      Change PROCEDURE to FUNCTION in CREATE EVENT TRIGGER syntax · cf3dfea4
      Peter Eisentraut authored
      This was claimed to have been done in
      0a63f996, but that actually only
      changed the documentation and not the grammar.  (That commit did fully
      change it for CREATE TRIGGER.)
      cf3dfea4
    • Tom Lane's avatar
      Add assertions that we hold some relevant lock during relation open. · b04aeb0a
      Tom Lane authored
      Opening a relation with no lock at all is unsafe; there's no guarantee
      that we'll see a consistent state of the relevant catalog entries.
      While use of MVCC scans to read the catalogs partially addresses that
      complaint, it's still possible to switch to a new catalog snapshot
      partway through loading the relcache entry.  Moreover, whether or not
      you trust the reasoning behind sometimes using less than
      AccessExclusiveLock for ALTER TABLE, that reasoning is certainly not
      valid if concurrent users of the table don't hold a lock corresponding
      to the operation they want to perform.
      
      Hence, add some assertion-build-only checks that require any caller
      of relation_open(x, NoLock) to hold at least AccessShareLock.  This
      isn't a full solution, since we can't verify that the lock level is
      semantically appropriate for the action --- but it's definitely of
      some use, because it's already caught two bugs.
      
      We can also assert that callers of addRangeTableEntryForRelation()
      hold at least the lock level specified for the new RTE.
      
      Amit Langote and Tom Lane
      
      Discussion: https://postgr.es/m/16565.1538327894@sss.pgh.pa.us
      b04aeb0a
    • Tom Lane's avatar
      Fix tuple_data_split() to not open a relation without any lock. · b66827ca
      Tom Lane authored
      contrib/pageinspect's tuple_data_split() function thought it could get
      away with opening the referenced relation with NoLock.  In practice
      there's no guarantee that the current session holds any lock on that
      rel (even if we just read a page from it), so that this is unsafe.
      
      Switch to using AccessShareLock.  Also, postpone closing the relation,
      so that we needn't copy its tupdesc.  Also, fix unsafe use of
      att_isnull() for attributes past the end of the tuple.
      
      Per testing with a patch that complains if we open a relation without
      holding any lock on it.  I don't plan to back-patch that patch, but we
      should close the holes it identifies in all supported branches.
      
      Discussion: https://postgr.es/m/2038.1538335244@sss.pgh.pa.us
      b66827ca
    • Tom Lane's avatar
      Fix ALTER COLUMN TYPE to not open a relation without any lock. · e27453bd
      Tom Lane authored
      If the column being modified is referenced by a foreign key constraint
      of another table, ALTER TABLE would open the other table (to re-parse
      the constraint's definition) without having first obtained a lock on it.
      This was evidently intentional, but that doesn't mean it's really safe.
      It's especially not safe in 9.3, which pre-dates use of MVCC scans for
      catalog reads, but even in current releases it doesn't seem like a good
      idea.
      
      We know we'll need AccessExclusiveLock shortly to drop the obsoleted
      constraint, so just get that a little sooner to close the hole.
      
      Per testing with a patch that complains if we open a relation without
      holding any lock on it.  I don't plan to back-patch that patch, but we
      should close the holes it identifies in all supported branches.
      
      Discussion: https://postgr.es/m/2038.1538335244@sss.pgh.pa.us
      e27453bd
    • Peter Eisentraut's avatar
      doc: Clarify CREATE TABLESPACE documentation · a6949ca3
      Peter Eisentraut authored
      Be more specific about when and how to create the directory and what
      permissions it should have.
      
      Discussion: https://www.postgresql.org/message-id/flat/5ca60e1a-26f9-89fd-e912-021dd2b8afe2%40gmail.com
      a6949ca3
  4. 30 Sep, 2018 1 commit
    • Tom Lane's avatar
      Create an RTE field to record the query's lock mode for each relation. · fdba460a
      Tom Lane authored
      Add RangeTblEntry.rellockmode, which records the appropriate lock mode for
      each RTE_RELATION rangetable entry (either AccessShareLock, RowShareLock,
      or RowExclusiveLock depending on the RTE's role in the query).
      
      This patch creates the field and makes all creators of RTE nodes fill it
      in reasonably, but for the moment nothing much is done with it.  The plan
      is to replace assorted post-parser logic that re-determines the right
      lockmode to use with simple uses of rte->rellockmode.  For now, just add
      Asserts in each of those places that the rellockmode matches what they are
      computing today.  (In some cases the match isn't perfect, so the Asserts
      are weaker than you might expect; but this seems OK, as per discussion.)
      
      This passes check-world for me, but it seems worth pushing in this state
      to see if the buildfarm finds any problems in cases I failed to test.
      
      catversion bump due to change of stored rules.
      
      Amit Langote, reviewed by David Rowley and Jesper Pedersen,
      and whacked around a bit more by me
      
      Discussion: https://postgr.es/m/468c85d9-540e-66a2-1dde-fec2b741e688@lab.ntt.co.jp
      fdba460a
  5. 28 Sep, 2018 9 commits
  6. 27 Sep, 2018 5 commits
    • Tom Lane's avatar
      Fix assorted bugs in pg_get_partition_constraintdef(). · aaf10f32
      Tom Lane authored
      It failed if passed a nonexistent relation OID, or one that was a non-heap
      relation, because of blindly applying heap_open to a user-supplied OID.
      This is not OK behavior for a SQL-exposed function; we have a project
      policy that we should return NULL in such cases.  Moreover, since
      pg_get_partition_constraintdef ought now to work on indexes, restricting
      it to heaps is flat wrong anyway.
      
      The underlying function generate_partition_qual() wasn't on board with
      indexes having partition quals either, nor for that matter with rels
      having relispartition set but yet null relpartbound.  (One wonders
      whether the person who wrote the function comment blocks claiming that
      these functions allow a missing relpartbound had ever tested it.)
      
      Fix by testing relispartition before opening the rel, and by using
      relation_open not heap_open.  (If any other relkinds ever grow the
      ability to have relispartition set, the code will work with them
      automatically.)  Also, don't reject null relpartbound in
      generate_partition_qual.
      
      Back-patch to v11, and all but the null-relpartbound change to v10.
      (It's not really necessary to change generate_partition_qual at all
      in v10, but I thought s/heap_open/relation_open/ would be a good
      idea anyway just to keep the code in sync with later branches.)
      
      Per report from Justin Pryzby.
      
      Discussion: https://postgr.es/m/20180927200020.GJ776@telsasoft.com
      aaf10f32
    • Alexander Korotkov's avatar
      Minor formatting cleanup for 2a636834 · 4ec90f53
      Alexander Korotkov authored
      4ec90f53
    • Alexander Korotkov's avatar
    • Andres Freund's avatar
      Clean up in the wake of TupleDescGetSlot() removal / 10763358. · 27e082b0
      Andres Freund authored
      The previous commit wasn't careful enough to remove all traces of
      TupleDescGetSlot().
      
      Besides fixing the oversight of not removing TupleDescGetSlot()'s
      declaration, this also removes FuncCallContext->slot. That was
      documented to be for use in combination with TupleDescGetSlot(), a
      cursory search over extensions finds no users, and there doesn't seem
      to be convincing reasons to keep it around. If we later in the v12
      release cycle find users, we can re-consider this part of the commit.
      
      Reported-By: Michael Paquier
      Discussion: https://postgr.es/m/20180926000413.GC1659@paquier.xyz
      27e082b0
    • Tom Lane's avatar
      Build src/port files as a library with -fPIC, and use that in libpq. · ea53100d
      Tom Lane authored
      libpq and ecpg need shared-library-friendly versions of assorted src/port/
      and src/common/ modules.  Up to now, they got those by symlinking the
      individual source files and compiling them locally.  That's baroque, and a
      pain to maintain, and it results in some amount of duplicated compile work.
      It might've made sense when only a couple of files were needed, but the
      list has grown and grown and grown :-(
      
      It makes more sense to have the originating directory build a third variant
      of libpgport.a/libpgcommon.a containing modules built with $(CFLAGS_SL),
      and just link that into the shared library.  Unused files won't get linked,
      so the end result should be the same.
      
      This patch makes a down payment on that idea by having src/port/ build
      such a library and making libpq use it.  If the buildfarm doesn't expose
      fatal problems with the approach, I'll extend it to the other cases.
      
      Discussion: https://postgr.es/m/13022.1538003440@sss.pgh.pa.us
      ea53100d
  7. 26 Sep, 2018 11 commits
    • Tom Lane's avatar
      Fix another portability issue from commit 758ce9b7. · ce4887bd
      Tom Lane authored
      strerror.c now requires strlcpy() in some cases, and a couple of the
      ecpg libraries did not have that at hand.  Pull it in from src/port/
      following the usual recipe.  Per buildfarm.
      ce4887bd
    • Michael Paquier's avatar
      Switch flags tracking pending interrupts to sig_atomic_t · ba16aade
      Michael Paquier authored
      Those previously used bool, which should be safe on any modern
      platforms, however the C standard is clear that it is better to use
      sig_atomic_t for variables manipulated in signal handlers.  This commit
      adds at the same time PGDLLIMPORT to ClientConnectionLost.
      
      Author: Michael Paquier
      Reviewed-by: Tom Lane, Chris Travers, Andres Freund
      Discussion: https://postgr.es/m/20180925011311.GD1354@paquier.xyz
      ba16aade
    • Tom Lane's avatar
      Try another way to detect the result type of strerror_r(). · 751f532b
      Tom Lane authored
      The method we've traditionally used, of redeclaring strerror_r() to
      see if the compiler complains of inconsistent declarations, turns out
      not to work reliably because some compilers only report a warning,
      not an error.  Amazingly, this has gone undetected for years, even
      though it certainly breaks our detection of whether strerror_r
      succeeded.
      
      Let's instead test whether the compiler will take the result of
      strerror_r() as a switch() argument.  It's possible this won't
      work universally either, but it's the best idea I could come up with
      on the spur of the moment.
      
      We should probably back-patch this once the dust settles, but
      first let's see what the buildfarm thinks of it.
      
      Discussion: https://postgr.es/m/10877.1537993279@sss.pgh.pa.us
      751f532b
    • Tom Lane's avatar
      Clean up *printf macros to avoid conflict with format archetypes. · 8b91d258
      Tom Lane authored
      We must define the macro "printf" with arguments, else it can mess
      up format archetype attributes in builds where PG_PRINTF_ATTRIBUTE
      is just "printf".  Fortunately, that's easy to do now that we're
      requiring C99; we can use __VA_ARGS__.
      
      On the other hand, it's better not to use __VA_ARGS__ for the rest
      of the *printf crew, so that one can take the addresses of those
      functions without surprises.
      
      I'd proposed doing this some time ago, but forgot to make it happen;
      buildfarm failures subsequent to 96bf88d5 reminded me.
      
      Discussion: https://postgr.es/m/22709.1535135640@sss.pgh.pa.us
      Discussion: https://postgr.es/m/20180926190934.ea4xvzhkayuw7gkx@alap3.anarazel.de
      8b91d258
    • Tom Lane's avatar
      Fix link failures due to snprintf/strerror changes. · a6b88d68
      Tom Lane authored
      snprintf.c requires isnan(), which requires -lm on some platforms.
      libpq never bothered with -lm before, but now it needs it.
      
      strerror.c tries to translate a string or two, which requires -lintl.
      We'd managed never to need that anywhere in ecpg/pgtypeslib/ before,
      but now we do.
      
      Per buildfarm and a report from Peter Eisentraut.
      
      Discussion: https://postgr.es/m/20180926190934.ea4xvzhkayuw7gkx@alap3.anarazel.de
      Discussion: https://postgr.es/m/f67b5008-9f01-057f-2bff-558cb53af851@2ndquadrant.com
      a6b88d68
    • Peter Eisentraut's avatar
      Recurse to sequences on ownership change for all relkinds · 0320ddaf
      Peter Eisentraut authored
      When a table ownership is changed, we must apply that also to any owned
      sequences.  (Otherwise, it would result in a situation that cannot be
      restored, because linked sequences must have the same owner as the
      table.)  But this was previously only applied to regular tables and
      materialized views.  But it should also apply to at least foreign
      tables.  This patch removes the relkind check altogether, because it
      doesn't save very much and just introduces the possibility of similar
      omissions.
      
      Bug: #15238
      Reported-by: default avatarChristoph Berg <christoph.berg@credativ.de>
      0320ddaf
    • Tom Lane's avatar
      Implement %m in src/port/snprintf.c, and teach elog.c to rely on that. · d6c55de1
      Tom Lane authored
      I started out with the idea that we needed to detect use of %m format specs
      in contexts other than elog/ereport calls, because we couldn't rely on that
      working in *printf calls.  But a better answer is to fix things so that it
      does work.  Now that we're using snprintf.c all the time, we can implement
      %m in that and we've fixed the problem.
      
      This requires also adjusting our various printf-wrapping functions so that
      they ensure "errno" is preserved when they call snprintf.c.
      
      Remove elog.c's handmade implementation of %m, and let it rely on
      snprintf to support the feature.  That should provide some performance
      gain, though I've not attempted to measure it.
      
      There are a lot of places where we could now simplify 'printf("%s",
      strerror(errno))' into 'printf("%m")', but I'm not in any big hurry
      to make that happen.
      
      Patch by me, reviewed by Michael Paquier
      
      Discussion: https://postgr.es/m/2975.1526862605@sss.pgh.pa.us
      d6c55de1
    • Tom Lane's avatar
      Always use our own versions of *printf(). · 96bf88d5
      Tom Lane authored
      We've spent an awful lot of effort over the years in coping with
      platform-specific vagaries of the *printf family of functions.  Let's just
      forget all that mess and standardize on always using src/port/snprintf.c.
      This gets rid of a lot of configure logic, and it will allow a saner
      approach to dealing with %m (though actually changing that is left for
      a follow-on patch).
      
      Preliminary performance testing suggests that as it stands, snprintf.c is
      faster than the native printf functions for some tasks on some platforms,
      and slower for other cases.  A pending patch will improve that, though
      cases with floating-point conversions will doubtless remain slower unless
      we want to put a *lot* of effort into that.  Still, we've not observed
      that *printf is really a performance bottleneck for most workloads, so
      I doubt this matters much.
      
      Patch by me, reviewed by Michael Paquier
      
      Discussion: https://postgr.es/m/2975.1526862605@sss.pgh.pa.us
      96bf88d5
    • Tom Lane's avatar
      Incorporate strerror_r() into src/port/snprintf.c, too. · 758ce9b7
      Tom Lane authored
      This provides the features that used to exist in useful_strerror()
      for users of strerror_r(), too.  Also, standardize on the GNU convention
      that strerror_r returns a char pointer that may not be NULL.
      
      I notice that libpq's win32.c contains a variant version of strerror_r
      that probably ought to be folded into strerror.c.  But lacking a
      Windows environment, I should leave that to somebody else.
      
      Discussion: https://postgr.es/m/2975.1526862605@sss.pgh.pa.us
      758ce9b7
    • Tom Lane's avatar
      Convert elog.c's useful_strerror() into a globally-used strerror wrapper. · 26e9d4d4
      Tom Lane authored
      elog.c has long had a private strerror wrapper that handles assorted
      possible failures or deficiencies of the platform's strerror.  On Windows,
      it also knows how to translate Winsock error codes, which the native
      strerror does not.  Move all this code into src/port/strerror.c and
      define strerror() as a macro that invokes it, so that both our frontend
      and backend code will have all of this behavior.
      
      I believe this constitutes an actual bug fix on Windows, since AFAICS
      our frontend code did not report Winsock error codes properly before this.
      However, the main point is to lay the groundwork for implementing %m
      in src/port/snprintf.c: the behavior we want %m to have is this one,
      not the native strerror's.
      
      Note that this throws away the prior use of src/port/strerror.c,
      which was to implement strerror() on platforms lacking it.  That's
      been dead code for nigh twenty years now, since strerror() was
      already required by C89.
      
      We should likewise cause strerror_r to use this behavior, but
      I'll tackle that separately.
      
      Patch by me, reviewed by Michael Paquier
      
      Discussion: https://postgr.es/m/2975.1526862605@sss.pgh.pa.us
      26e9d4d4
    • Peter Eisentraut's avatar
      Update dummy CREATE ASSERTION grammar · a49ceda6
      Peter Eisentraut authored
      While we are probably still far away from fully implementing
      assertions, all patch proposals appear to take issue with the existing
      dummy grammar CREATE/DROP ASSERTION productions, so update those a
      little bit.  Rename the rule, use any_name instead of name, and remove
      some unused code.  Also remove the production for DROP ASSERTION,
      since that would most likely be handled via the generic DROP support.
      
      extracted from a patch by Joe Wildish
      a49ceda6