So we have a problem which is triggered in part by the race detector,
but which is actually deeper, but also possibly rare enough to be
politely ignored.
The real underlying issue is that sometimes when we have multiple
tests running in CI, multiple instances of the CLI test end up using
the same postgres database backing for some of their DAX stuff. We
have workarounds for this in some places, but not others.
But the *observed symptom* of this is that it can cause a trivial
race detector issue where we have one call to `(*Resource).Lock()`
and another call to `(*Resource).IsLocked()` which aren't synchronized
in any way, so if the race detector spots this, it complains.
We can suppress that very easily by synchronizing these. That does
not solve the other possibly-weird problems, so this may not actually
address the issue, but I think it might reduce the rate of sporadic
failures significantly, which would give us some time to think about
solving the deeper problem.
The underlying design issue is that we're reusing the database name
in postgres for testing. This lets us have bounded growth (one database)
while leaving the database contents up after a failed test (so we can
examine them), then truncating the database during startup if it already
exists. Which works fine if *only one thing runs at once*, which would
be true on a laptop, but in CI, it's sometimes not true. A real fix
for that is complex and requires some rethinking of how we approach
the test stuff, as we don't want unbounded growth, but we also don't
want two copies of the test running at once to see each other, and
ensuring cleanup after a test failure is surprisingly hard.
There's a lot going on here. First, we were treating "the test is
a Condition" as implying BSI, which it doesn't anymore. Second, the
behavior of conditions was weird and BSI-specific. Third, we had
to propagate these changes and features throughout a bunch of code,
including both the core featurebase code and the DAX replacements/copies
of it, plus the SQL3 layer.
We refactor this so that tests for equality and inequality work for
non-BSI fields, so now if you accidentally use `==` in a Row call
on a non-BSI field, it still works; that's not specific to BSI
fields anymore.
We add a TrackExistence flag to fields, and propagate it through
things like our protobuf code, etcetera, so that we can successfully
create fields. Newly-created fields get this by default, because
we add it unconditionally to them, but the paths that are being
called with existing fields don't add it. So, when we "create"
(really, just load the definition of) a field from something stored
in the schema, we don't add TrackExistence to it, but any path to
creating a new field should.
A time quantum field with NoStandardView will *effectively*
lack TrackExistence.
For sets, mutexes, and time quantums with a standard view, anything
that sets bits will also set a corresponding bit for the record in
a new "existence" view. This allows us to distinguish between an
empty set and a null, and also allows null checks to be constant-time.
When clearing bits, we don't clear existence bits EXCEPT that if
you clear a bit in a mutex, *and the bit actually existed*, we clear
the existence bit. For sets and time quantums, clearing bits never
clears the existence bit.
Deleting records clears the existence bit.
We also add code to the `batch` subpackage to generate suitable
existence field bitmaps and import them. This logic correctly handles
empty sets and nils. The `batch` package does not allow specification
of anything equivalent to clearing a single bit from an existing
record, so we don't have to deal with the mutex complexity in that
case, which is good because it would be impossible.
This requires a number of other subtle changes, such as allowing
new fields to have more than one FieldOption specified for them.
We also drop the handful of implementation bits relating to the
"fullySorted" internal-use-only import flag, which existed only to
support the JSON ingest API, which we've removed.
The most dangerous part of this is that the mutex semantics are
impossible to implement on top of our existing API, because they
require us to know, not how *many* bits we cleared, but which
*specific* bits we cleared. I've implemented this as a new Tx method,
which is almost certainly going to be tech debt one day; if we some
day drop the Import API, we should remove that.
The testing for this is only currently covering the Set/Clear
behavior of PQL, and the Import API. The batch tests haven't been
written yet.
Fields that don't have existence tracking enabled refuse to perform
null/not-null tests. They should also report themselves as having
no null values -- if a record exists, sets in it are considered
empty rather than null.
The SQL3 support requires a number of subtle modifications to both
featurebase and some addon tooling. The essential thing is dropping
the unconditional translation of nil slices to non-nil empty slices
in translateResult, both in the executor and the orchestrator. We
also modify the logic that handles generating results from Extract
calls, to ensure that non-null sets get an empty slice created for
them even if they never have any values assigned.
The expected results for some tests are different now; we expect to
get nil slices, rather than 0-length non-nil slices, for fields which
were never written for a given record. Most tests were not changed.
(In every case, if a test was failing, I actually checked the logic
before changing expected results. This required a lot of tracking down
of edge cases.)
The batch package now rejects as an error attempts to clear single
bits from mutex fields, because so far as I can tell it's simply
impossible to have a roaring import that specifies the correct semantics
there; you can't tell whether to clear an existence bit without
access to the currently-set bits, which the batch API doesn't have.
We already supported the special case of specifying a clear value
of nil for clearing a mutex field; now that is the only allowed
value for a mutex field to have in row.Clears.
We change the logic for fixing up incoming view names (in two places)
to stop assuming that any view in a time field other than "" that does
not have viewStandard as a prefix is a partial time quantum name that
should have "standard_" prepended to it. This allows us to submit
bitmaps for "existence" to time quantum fields and not have them
silently transformed into "standard_existence" because that's what we'd
do with "202203".
We drop the field ClearBits method, which was totally unused.
We drop the sliceDifference function, which was used in a previous
mutex implementation and hasn't been used in ages, and the test
case for it, and the helper function used only by that test case.
* added dupe check
* added dupe check for databases
* prevent dupe table names
* refactored table dupe search
* further refactoring
This commit, further refactoring of checking for duplicates are done with SQL commands
Also adjusted tests to confirm changes
* fixed linting errors
* moved the errors around to keep them consolidated in the dax package
also removed useless comment
FB-2041
Added tests for typeIsTimeQuantum and typeIsSet and made them pass.
Added tests for DataTypeTuple cases in typesAreAssignmentCompatible.
Timestamp conversion checking is handled before it gets to that
point but I left those branches in as a backstop.
Checking to see if DataType[String,ID]SetQuantum can be assigned
to themselves doesn't appear to be reachable currently but left
those branches in, because something may use them in future.
Added one test to the DAX skip list since it's the IDSetQ version
of a StringSetQ test that was already on there, changed skip list
to refer to both tests by name instead of by number.
* serverless sqldb use same env for test config as normal
* rip boltdb implementation of controller backend out
it was replaced by postgres and no longer works properly.
This involved migrating a number of tests which only worked with
boltdb, which exposed several ways in which the postgres
implementation had slightly different behavior from the bolt
one:
1. ordering of results in some cases, and
2. (more importantly) erroring when a record to delete was not
found. The bolt implementation silently ignored it when things to
delete weren't found, so we make some changes to match that behavior.
Also stopped propagating CreatedAt and UpdatedAt from DB tables into
dax types. These were breaking existing tests. Perhaps it would be
better to actually use them, but for now they will only exist at the
DB level.
This change set also moves the insertion of the directive_versions
record out of migrations and into the startup/connection code. Having
this in the migrations was a bit ugly because you couldn't just
truncate all the tables and have everything work from
scratch. Inserting it during startup is fairly innocuous, and will
just continue on if it already exists.
* update directive_version test
I changed the initial value to 0 so that the first version that gets
sent out is 1
For on-prem serverless, if we restart the process containing the
controller and computer(s), when they come back up, the controller
doesn't know that the computers have been restarted, so it doesn't send
them a directive. This change forces the controller to send a directive
upon startup by a computer.
Switch Serverless from using BoltDB to Postgres as metadata store.
Previously, the controller stored all metadata to BoltDB. This implements SQLDB (currently Postgres flavored) as the backing store for metadata. This will allow us to have multiple instances of the controller running for HA, and to easily inspect and repair the contents of the metadata store.
Unfortunately, it was not straightforward to keep the BoltDB implementation working alongside the SQL one, so it will be removed in a later patch. Once that's done, the SQL implementation should allow for a number of simplifications of the schemar and balancer interfaces.
Database migration is built directly into the application by embedding the migration files and logic from the `soda` command line tool. When connecting to the RDBMS, the app will always attempt to create the necessary database and apply any outstanding migrations.
Integration tests truncate all tables upon start, but *not* at the end, so the state of the database can be inspected after integration tests.
Had to refactor some of the controller's background tasks to make sure they get properly shut down on controller exit.
* Enable linter: stylecheck
This enabled the stylecheck linter, but excludes some staticchecks for
now. The following are ignored because they will take a bit of time to
address, but the intention is to address them and remove them from the
exclusion list.
ST1000: at least one file in a package should have a package comment
ST1003: golang naming standards
ST1008: error should be returned as the last argument
ST1016: methods on the same type should have the same receiver name
ST1020: comment on exported function
* Address ST1015
For some reason this failed in CI but not locally. I can't figure out
why that check isn't happening locally. This just moves the switch
statements around so that the `default` is the first (or last) item.
* Adjust error string in test to match case-adjusted error
* Remove TestCloseTimeout
* Add (commented out) linters that we should introduce
I went through the available linters and added (commented out) the ones
I think we should work on in the near term. In other words, fix them,
then uncomment them so they are enabled in CI.
* linter: errchkjson
* linter: ineffassign
* linter: gosimple
* linter: errname
* Add kafka support to CLI (fbsql)
This commit adds the ability to provide a `--kafka-config` command line
argument referncing a toml file to configure kafka.
* Move "Molecula Consumer" message to the logger; hide it in basic mode
* Fold decimal(scale) into kafka.source-type
* Build fbsql with cgo in docker for CI
* Re-organize the fbsql kafka config and setup.
Allow field config to use the table schema if no fields provided.
* Display timestamp fields with format RFC3339Nano
* remove kafkaRunner (no longer used)
* Fix cli/batch test (and make sure it's not excluded from CI)
The logic in our Makefile was exluding from tests any package with
`/batch` in the package name. This excluded `/cli/batch`, which is not
good.
This commit changes the exclusion logic to include the `/v3` portion of
the package name, so `/v3/batch`.
* Rename Basic() to SetBasic()
Buckle in, this one's a ride.
This is attached to the same PR as a fix for exiting abruptly
during some tests because I ran into that issue, and comprehended
it, while trying to track down weird and sporadic test failures
that were actually this issue.
The actual, underlying, problem: `make test`, by running all the
tests at once, was hitting a bug that was mostly effectively
triggered by running the `dax/test/dax` tests, and the top-level
`featurebase/v3` tests, at the same time. However, the interaction
was nothing as obvious as temporary files, etcd configuration,
or whatever.
We were running out of port numbers.
The tests were using a bit over 30k simultaneous established TCP
connections, each to different ports, because we were creating
new clients for basically every single operation. For instance,
in a single SQL test that did an import and then a read, we
were creating a new client for each field written to, and then
also creating a new client for each field in results that needed
key translation. And none of these clients were closed or
timed out in any way. In fact, Go doesn't really *do* "closing"
of clients; the closest is that an http.Client can be told to
close idle connections that it has been keeping open.
The worst offenders were both named `fbClient`, and were nigh-identical,
except one of them was implemented as a method on `importer` in the
IDK tree, and one was a standalone function.
It may seem surprising that the method on `importer` is using a shared
client pool for all importers, rather than a new pool for each
importer. This is because we potentially make quite a few importers
during tests.
Before this, running either of the dax tests or the top-level
tests would show well over ten thousand simultaneous ESTABLISHED
connections. After this, the dax tests used nearly twenty.
The problem with port consumption like this, while more noticeable
on MacOS, is also something we could hit on the CI runners, especially
if a single runner ended up with more than one test suite running
at the same time. This probably manifests as sporadic very strange
failures of CI, with messages about "cannot assign requested address".
(Note that an outgoing connection to a successfully-created port
requires *another* port to be assigned for the outbound socket.)
This was complicated dramatically by the fact that, for some
utterly cursed reason, it was *especially* common for the point
at which we hit this, in the top-level featurebase tests, to be
running one of the backup tests in TestVariousQueries, and
specifically, to be hitting it on the dataframe part of the
backup... Which is to say, on the *one* path in the backup function
that called log.Fatal, and thus terminated the featurebase process
abruptly without further commentary.
* first time quantum queries working
* implement select from timequantum columns
* skip a dax test
* make linter happy
* addressed review feedback
* reverted over eager test elimination
This forward-ports a number of tests from the previous SQL
implementation. The porting is approximate in a number of ways,
and not all tests are implemented/tested yet.
In particular, several tests are currently disabled because
we don't support `limit n` constructs.
The tests that were primarily tests of the parser have been
brought forward as parser tests. One of them has been altered
to add parentheses, because our parser interprets
fld1 between 1 and 3 and fld2 = 2
as:
fld1 between (1 and 3) and (fld2 = 2)
which is invalid, while the old parser apparently interpreted it
as:
(fld1 between 1 and 3) and (fld2 = 2)
We have not yet verified the SQL spec's requirements here, but
sqlite agrees with our old parser, not our new parser, so this
may be a regression.
The old tests expected an INNER JOIN to suppress duplicate
values. Our new code does not, which is consistent with other
SQL implementations. This is a change, but the old behavior
appears to have been wrong. (You can still suppress duplicate
values by specifying DISTINCT.)
In the previous implementations, a value like `count(*)` had
`count(*)` as its column name. In the new implementation,
it has an empty string as its column name.
Related to this, the prior implementation allowed you to
write
select age, count(*) from grouper group by age having count > 1
but the new implementationt requires that to be spelled as
having count(*) > 1
This is consistent with other SQL implementations, so I think
the new behavior is correct.
The behavior of SHOW COLUMNS and SHOW TABLES has changed, in
that the specific results returned are significantly different.
Perhaps more significantly, the old system spelled the former
query as SHOW FIELDS, rather than SHOW COLUMNS. This may be
considered a regression, in that `SHOW FIELDS` no longer works,
and we should consider whether any hypothetical users might
have been relying on the output of either of these. (I hope
not, the new output is much better.)
Some of the old tests (the ones in handler_test) were accommodated
by adding a couple of specific test cases to existing tests,
specifically:
* handling timestamp values with `Z` rather than `+00:00`
* a join with a WHERE clause referring to fields in both
source tables
We introduce a new "partial" comparison type, because there's
no way for a test of `SHOW TABLES` to contain a correct table
row, because `SHOW TABLES` includes timestamps from when tables
were created. I'm not sure this is the right way to do this.
We add corresponding changes to dax_test, because the DAX tree
tests against the SQL tests.
We change the returned types of field names and field types to
plain strings, ironically because DAX needs this -- the test code
in the DAX tree is getting them back as plain strings, rather
than as dax.FieldName and dax.BaseType.
The tests using `having` are commented out because they don't
seem to be working, a ticket has been filed for this.
Two of the tests that should return strings are instead returning
untranslated integer IDs, but only for DAX, not for the regular
SQL tests, and the `delete` test has been commented out for
DAX-specific errors. If we merge this, the next step is to
ticket those and address them separately.
* WIP: json marshal coded errors for http
* WIP: trying to see how best to implement the http-error tests
* finish stubbing out the Schemar methods in the test
* using json to move CodedErrors across boundaries and associated tests
* implemented feedback and fixes
---------
Co-authored-by: Travis Turner <travis@molecula.com>
* Removed support for EPOCH column constraint from TIMESTAMP SQL data type.
* Implicit conversion of integers to timestamp will treat the integer value as seconds since unix epoch.
* Add new ToTimeStamp(num, timeunit) SQL scalar function to help convert integer values to timestamp.
* Refactor CLI to mimic psql's meta-commands
This PR adds support for meta-commands (also known as "backslash
commands") like those in psql, Postgres's CLI. Only a few meta-commands
are currently implemented, but this was meant to demonstrate how we
could use something like `\i file.csv` to insert local files into SQL
statements.
* Meta-commands: \file and \include
The initial implementation used `\i` as a streaming file handle.
This commit changes that to `\file`, and then implements `\i` (or
`\include`) as handling multiple sql commands.
* Add meta-command "help" (\?)
This is basically a copy of the psql help output, but includes only
those options we currently support.
* Add support for \o [file], and \timing
The \o meta-command writes query output to a file.
The \timing meta-command turns on/off the timing display sent to stdout.
* Add meta-commands: \l (show databases) and \dt (show tables)
* Add meta-command: \watch [period]
* Update meta-command \connect to take database name instead of ID
* Add support for \echo, \qecho, and \warn
This commit contains an known issue in that the `-n` option will exclude
the line feed, but if the output is the terminal, the readline package
clobbers any content on the current line (i.e. anything without a line
feed). That will need to be addressed at some point.
* Add support for \w [FILE] (write query buffer to file)
* Add SchemaAPI no-op implementation
* Refactor query handler to align with /sql and /databases endpoints
We want to standardize on:
/sql
/databases/{databaseID}/sql
* Add CLI support for expanded, border, tuples_only (and pset)
* Add help text for \pset and \t
* Implement Schemar.SetDatabaseOption(option, value string)
This replaces the temporary `SetDatabaseOptions()` method, which
replaced the entire DatabaseOptions struct, with `SetDatabaseOption`
which takes an option/value pair of strings to set.
* Add SetDatabaseOption to controller http handler and client
This commit also:
- renames some `writeLog` to `writelog`
- updates ApplyDirective to call resource.Unlock() on any resources
being removed from the local worker
* Add Database related methods to SchemaAPI interface
Currently all implementations of this interface are implemented with
"unimplemented" errors on those methods. Next will be to implement the
necessary methods.
* SQL: CREATE DATABASE and SHOW DATABASES
* SQL: DROP DATABASE
* SQL: Add UNITS option to CREATE DATABASE
* SQL: ALTER DATABASE
* User serverlessStorage.Remove[*]Resource instead of resource.Unlock()
* Add WITH keyword to CREATE/ALTER DATABASE
* fix some WITH logic
* linter fixes
* WITH on CREATE DATABASE is not required
* tweak a bunch of logging and config
make overall logs less verbose and chatty
1 minute computer check-in interval
3 minute snapshot interval
remove CaptureLogger as it has same functionality as buffer logger
add a WithPrefix to the Logger interface so sub-services can have
different prefixes
* fix some lint
* fix lint... confused why this is coming up now
* Remove MDS and replace it with Controller
This commit removes the MDS layer (and package) and shifts Controller
package into its place.
* add pprof/fgprof to serverless http router
---------
Co-authored-by: Matthew Jaffee <jaffee@pilosa.com>
* Support Drop Table in serverless (include Snapshotter, Writelogger)
* Finish Database methods
Things like:
- `Databases`
- `DatabaseByID`
- `DatabaseByName`
- `DropDatabase`
* Change Poller to use NodeService instead of its own map
Instead of the Poller maintaining its own map of Addresses to poll, this
commit changes the Poller to use the NodeService interface to get all
known nodes from the Controller.
The next commit needs to:
Next, the logic in the boltdb NodeService implementation was moved to
the boltdb Balancer implementation. That way, the Balancer can be the
source of truth for all things nodes/workers/jobs.
* Move NodeService from Controller to Balancer
This commit moves the implementation of the NodeService into the
Balancer, and aligns `Balancer.AddWorker` with `NodeService.CreateNode`
so that they stay in sync. (Same for `Balancer.RemoveWorker` and
`NodeService.DeleteNode`).
* fix import of private repo
* Fix go vet issues
* Fix bug in DeregisterNode
We need to remove the node from the NodeService even if it's not
assigned to a database. The logic had a bug in it.
This also adds some no-op implementations for SnapshotService and
WriteloggerService. If a directory was not configured for that, then the
computer node would panic on trying to read from the Snapshotter upon
receiving a Directive.
* queryer response content-type: json
* Add support for NULL to WriteloggerDir and SnapshotterDir configs
This commit changes the way WriteLoggerDir and SnapshotterDir are
handled.
If value is empty `""`, an error will be returned on computer startup.
If value is `"NULL"`, a no-op implementation of the service will be
used. This would be for a case that wanted to run serverless on-prem
with no durable storage.
Finally, any other value will be used as the directory to use.
Some things which aren't considered here and may result in unexpected
behavior:
- a value with spaces `" "`
- any "null" which is not "NULL"... like lowercase.
* Finish the DropTable test
* Change "disable service" value to case-insensitive "off"
This commit also removes an unnecessary sleep in the tests.
* Fix docker-compose variables for IDK test
Co-authored-by: Matthew Jaffee <jaffee@pilosa.com>
* first cut at pre-sort command that works on ndjson
* finish pre_sort command for CSV and JSON and add test
* try fixing golangci-lint
* remove some dumb lint checks
* more linter disabling
* take .golangci.yml from previous repo
* go fmt (facepalm)
* remove ioutil to fix lint
* Database isolation: Balancer
Remove naive Balancer
remove debugging lines
Thread dax.Transaction through Controller
Change role to roleType
Swap out Balancer interface with new one
Standardize InvalidTransaction error
Add some interface comments
* Remove type.Worker; replace with type.Address
* Remove database validate from Queryer
This is already being handled in the `CreateTable()` method. Prior
to doing that validation, we were getting a panic, but that's no longer
the case.
* Remove dax.TableQualifier; replace with dax.QualifiedDatabaseID
* Update IDK test to create database
(cherry picked from commit d971cfc269)
* implement CREATE/ALTER/DROP VIEW
* fixed failing test
* another failing test
* fixed some broken serverless tests
(cherry picked from commit c620aae350)
* performance counters
* first cut of perf counters and system table fanout and a wire protocol
* significantly refactored prometheus support; removed statsd and exprvar
* removed node_id
* put dax subquery test back
* Change Translator.TranslateFieldIDs method to take a dax.TableKeyer
There are a bunch of other calls to the Translator interface methods
with currently take an `index string`, and those need to be converted to
dax.TableKeyer as well. But I need to review each call, because in at
least one place I noticed one being called with `result.Index` instead
of with the qtbl available. And I don't yet know how those could be
different.
Co-authored-by: Travis Turner <travis@molecula.com>
(cherry picked from commit 7f6ea0e6e5)
* handle sum aggregates with ints; handle escaped quotes in blob literals
* added test ceoverage
* skip subquery test for dax
(cherry picked from commit af475a27f2)
* implemented distinct
* implemented distinct
* uses first cut of a buffer pool, and extendible hashing with thresholded spill to disk
* tests
* cleaned up some stuff around query plan output to make developing tooling easier
* added optimization to call PQL Distinct()
* fixed test
* fix for passing wrong index name in orchestrator
* back out change to DistinctTimestamp
* fix other instance of wrong table name being passed
* use full index name instead of abbreviated one for translation. sigh.
* removed some unused code
Co-authored-by: Matthew Jaffee <jaffee@pilosa.com>
(cherry picked from commit f030d58d95)
- had to make sure we don't snapshot until directive is fully applied
on a computer... otherwise there's races between loading the files and
truncating the write log.
- added a dirty bit to resources and a bool return to incrementing the
write log... don't snapshot if it returns false because that means
there's been no writes. (but make sure you close the storage transaction!)
- added the actually snapshotting routine which just fires every
<timeout> and serially snapshots everything.
- tweaked some logging
- added ability to get all tables in an org/db or literally all. I
think I just needed the "literally all", but it was natural to allow
it to be scoped to org or DB as well.
(cherry picked from commit b8b08bc9eb)
* delete implementation with test coverage
* optimize IN expressions; stop linter complaining
* fixed some uncovered query cases
* skip test in DAX for now
(cherry picked from commit 021219935f)
* Fix PQL distinct in dax
When issuing a PQL Distinct() call (or any other call with a "index=" arg),
this commit will attempt to convert the value in the index arg with a
TableKeyer.
* Apply change to call.Children as well
* Add some PQL Distinct (join) test coverage
(cherry picked from commit 4e8fe488de)
- check that serverlessStorage is not nil before closing it
- check that we don't already hold a lock on a serverless storage
Manager before trying to load it. This fixed at least one test failure.
(cherry picked from commit 87d1c31607)
