1. 04 Oct, 2018 5 commits
  2. 03 Oct, 2018 8 commits
    • Andres Freund's avatar
    • Andres Freund's avatar
      Ensure that snprintf.c's fmtint() doesn't overflow when printing INT64_MIN. · 4868e446
      Andres Freund authored
      This isn't actually a live bug, as the output happens to be the
      same.  But it upsets tools like UBSan, which makes it worthwhile to
      fix.
      
      As it's an issue without practical consequences, don't backpatch.
      
      Author: Andres Freund
      Discussion: https://postgr.es/m/20180928001121.hhx5n6dsygqxr5wu@alap3.anarazel.de
      4868e446
    • Tom Lane's avatar
      Change executor to just Assert that table locks were already obtained. · 9a3cebea
      Tom Lane authored
      Instead of locking tables during executor startup, just Assert that
      suitable locks were obtained already during the parse/plan pipeline
      (or re-obtained by the plan cache).  This must be so, else we have a
      hazard that concurrent DDL has invalidated the plan.
      
      This is pretty inefficient as well as undercommented, but it's all going
      to go away shortly, so I didn't try hard.  This commit is just another
      attempt to use the buildfarm to see if we've missed anything in the plan
      to simplify the executor's table management.
      
      Note that the change needed here in relation_open() exposes that
      parallel workers now really are accessing tables without holding any
      lock of their own, whereas they were not doing that before this commit.
      This does not give me a warm fuzzy feeling about that aspect of parallel
      query; it does not seem like a good design, and we now know that it's
      had exactly no actual testing.  I think that we should modify parallel
      query so that that change can be reverted.
      
      Discussion: https://postgr.es/m/468c85d9-540e-66a2-1dde-fec2b741e688@lab.ntt.co.jp
      9a3cebea
    • Andres Freund's avatar
      Fix issues around EXPLAIN with JIT. · c03c1449
      Andres Freund authored
      I (Andres) was more than a bit hasty in committing 33001fd7
      after last minute changes, leading to a number of problems (jit output
      was only shown for JIT in parallel workers, and just EXPLAIN without
      ANALYZE didn't work).  Lukas luckily found these issues quickly.
      
      Instead of combining instrumentation in in standard_ExecutorEnd(), do
      so on demand in the new ExplainPrintJITSummary().
      
      Also update a documentation example of the JIT output, changed in
      52050ad8.
      
      Author: Lukas Fittl, with minor changes by me
      Discussion: https://postgr.es/m/CAP53PkxmgJht69pabxBXJBM+0oc6kf3KHMborLP7H2ouJ0CCtQ@mail.gmail.com
      Backpatch: 11, where JIT compilation was introduced
      c03c1449
    • Tom Lane's avatar
      Rationalize snprintf.c's handling of "ll" formats. · 595a0eab
      Tom Lane authored
      Although all known platforms define "long long" as 64 bits, it still feels
      a bit shaky to be using "va_arg(args, int64)" to pull out an argument that
      the caller thought was declared "long long".  The reason it was coded like
      this, way back in commit 3311c766, was to work around the possibility that
      the compiler had no type named "long long" --- and, at the time, that it
      maybe didn't have 64-bit ints at all.  Now that we're requiring compilers
      to support C99, those concerns are moot.  Let's make the code clearer and
      more bulletproof by writing "long long" where we mean "long long".
      
      This does introduce a hazard that we'd inefficiently use 128-bit arithmetic
      to convert plain old integers.  The way to tackle that would be to provide
      two versions of fmtint(), one for "long long" and one for narrower types.
      Since, as of today, no platforms require that, we won't bother with the
      extra code for now.
      
      Discussion: https://postgr.es/m/1680.1538587115@sss.pgh.pa.us
      595a0eab
    • Tom Lane's avatar
      Provide fast path in snprintf.c for conversion specs that are just "%s". · 6d842be6
      Tom Lane authored
      This case occurs often enough (around 45% of conversion specs executed
      in our regression tests are just "%s") that it's worth an extra test
      per conversion spec to allow skipping all the logic associated with
      field widths and padding when it happens.
      
      Discussion: https://postgr.es/m/26193.1538582367@sss.pgh.pa.us
      6d842be6
    • Tom Lane's avatar
      Make assorted performance improvements in snprintf.c. · abd9ca37
      Tom Lane authored
      In combination, these changes make our version of snprintf as fast
      or faster than most platforms' native snprintf, except for cases
      involving floating-point conversion (which we still delegate to
      the native sprintf).  The speed penalty for a float conversion
      is down to around 10% though, much better than before.
      
      Notable changes:
      
      * Rather than always parsing the format twice to see if it contains
      instances of %n$, do the extra scan only if we actually find a $.
      This obviously wins for non-localized formats, and even when there
      is use of %n$, we can avoid scanning text before the first % twice.
      
      * Use strchrnul() if available to find the next %, and emit the
      literal text between % escapes as strings rather than char-by-char.
      
      * Create a bespoke function (dopr_outchmulti) for the common case
      of emitting N copies of the same character, in place of writing
      loops around dopr_outch.
      
      * Simplify construction of the format string for invocations of sprintf
      for floats.
      
      * Const-ify some internal functions, and avoid unnecessary use of
      pass-by-reference arguments.
      
      Patch by me, reviewed by Andres Freund
      
      Discussion: https://postgr.es/m/11787.1534530779@sss.pgh.pa.us
      abd9ca37
    • Amit Kapila's avatar
      MAXALIGN the target address where we store flattened value. · 9bc9f72b
      Amit Kapila authored
      The API (EOH_flatten_into) that flattens the expanded value representation
      expects the target address to be maxaligned.  All it's usage adhere to that
      principle except when serializing datums for parallel query.  Fix that
      usage.
      
      Diagnosed-by: Tom Lane
      Author: Tom Lane and Amit Kapila
      Backpatch-through: 9.6
      Discussion: https://postgr.es/m/11629.1536550032@sss.pgh.pa.us
      9bc9f72b
  3. 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
  4. 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
  5. 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
  6. 28 Sep, 2018 9 commits
  7. 27 Sep, 2018 4 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